* Re: [PATCH] git-gui: fix multi selected file operation
From: Pat Thoyts @ 2011-10-17 22:26 UTC (permalink / raw)
To: Bert Wesarg; +Cc: git
In-Reply-To: <CAKPyHN0iaS301r_d+kc-AVRNVKUprhdMwDpdD0HDf7nKbsPR3Q@mail.gmail.com>
Bert Wesarg <bert.wesarg@googlemail.com> writes:
>Hi,
>
>On Sun, Oct 16, 2011 at 00:48, Pat Thoyts
><patthoyts@users.sourceforge.net> wrote:
>> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>>
>>>The current path for what we see the diff is not in the list of selected
>>>paths. But when we add single paths (with Ctrl) to the set the current path
>>>would not be used when the action is performed.
>>>
>>>Fix this by explicitly putting the path into the list before we start
>>>showing the diff.
>>>
>>>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>>>---
>>> git-gui.sh | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>>>
>>>diff --git a/git-gui.sh b/git-gui.sh
>>>index f897160..e5dd8bc 100755
>>>--- a/git-gui.sh
>>>+++ b/git-gui.sh
>>>@@ -2474,6 +2474,7 @@ proc toggle_or_diff {w x y} {
>>> [concat $after [list ui_ready]]
>>> }
>>> } else {
>>>+ set selected_paths($path) 1
>>> show_diff $path $w $lno
>>> }
>>> }
>>
>> It is not clear what I should be looking for to test this. Can you
>> re-write the commit message to be more clear about what you are
>> fixing. Is this multiple unstaged files in the staging box? If so I
>> don't see what path display is changing.
>
>Sorry, for this bad description. I will give you a recipe here what to
>do to expose the problem. I try later to form this into a new commit
>message:
>
>You have 2 modified, not staged files A and B. Your current view shows
>the diff for A. Adding B to the selection via Ctrl+Button1 and than
>perform the "Stage To Commit" action from the "Commit" menu results
>only in the staging of B.
>
>Note, using Shift+Button1 (i.e. 'adding a range of files to the
>selection') results in the staging of both files A and B.
>
>Bert
Ah ok - that explains things and I can see the issue now. I think
something like:
"When staging a selection of files using Shift-Click to choose a range
of files then using Ctrl-T or the Stage To Commit menu item will stage
all the selected files. However if a non-sequential range is selected
using Ctrl-Click then only the last name selected gets staged. This
commit fixes this to properly stage all selected files by explicitly
adding the path to the list before showing the diff."
will do.
--
Pat Thoyts http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97 10 CE 11 E6 04 E0 B9 DD
^ permalink raw reply
* Re: [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:21 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster
In-Reply-To: <1318889937-17693-1-git-send-email-andreiw@vmware.com>
----- Original Message -----
> From: "Andrei Warkentin" <andreiw@vmware.com>
> To: git@vger.kernel.org, gitster@pobox.com
> Cc: "Andrei Warkentin" <andreiw@vmware.com>
> Sent: Monday, October 17, 2011 6:18:57 PM
> Subject: [PATCH] Git-p4: Add "git p4 change" command.
>
> Many users of p4/sd use changelists for review, regression
> tests and batch builds.
>
> "p4 change" is almost equivalent to "p4 submit", yet will
> just create the changelist and not submit it.
>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
> contrib/fast-import/git-p4 | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..dd084b9 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -950,7 +950,10 @@ class P4Submit(Command, P4UserMap):
> if checkModTime and (os.stat(fileName).st_mtime <=
> mtime):
> response = "x"
> while response != "y" and response != "n":
> - response = raw_input("Submit template unchanged.
> Submit anyway? [y]es, [n]o (skip this patch) ")
> + if (self.cmdname == "change"):
> + response = raw_input("Change template
> unchanged. Create changelist anyway? [y]es, [n]o (skip this patch)
> ")
> + else:
> + response = raw_input("Submit template
> unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
>
> if response == "y":
> tmpFile = open(fileName, "rb")
> @@ -959,7 +962,10 @@ class P4Submit(Command, P4UserMap):
> submitTemplate =
> message[:message.index(separatorLine)]
> if self.isWindows:
> submitTemplate = submitTemplate.replace("\r\n",
> "\n")
> - p4_write_pipe("submit -i", submitTemplate)
> + if (self.cmdname == "change"):
> + p4_write_pipe("change -i", submitTemplate)
> + else:
> + p4_write_pipe("submit -i", submitTemplate)
>
> if self.preserveUser:
> if p4User:
> @@ -981,9 +987,14 @@ class P4Submit(Command, P4UserMap):
> file = open(fileName, "w+")
> file.write(self.prepareLogMessage(template, logMessage))
> file.close()
> - print ("Perforce submit template written as %s. "
> - + "Please review/edit and then use p4 submit -i <
> %s to submit directly!"
> - % (fileName, fileName))
> + if (self.cmdname == "change"):
> + print ("Perforce change template written as %s. "
> + + "Please review/edit and then use p4 change
> -i < %s to submit directly!"
> + % (fileName, fileName))
> + else:
> + print ("Perforce submit template written as %s. "
> + + "Please review/edit and then use p4 submit
> -i < %s to submit directly!"
> + % (fileName, fileName))
>
> def run(self, args):
> if len(args) == 0:
> @@ -2177,6 +2188,7 @@ commands = {
> "debug" : P4Debug,
> "submit" : P4Submit,
> "commit" : P4Submit,
> + "change" : P4Submit,
> "sync" : P4Sync,
> "rebase" : P4Rebase,
> "clone" : P4Clone,
> @@ -2202,6 +2214,7 @@ def main():
> sys.exit(2)
>
> options = cmd.options
> + cmd.cmdname = cmdName
> cmd.gitdir = os.environ.get("GIT_DIR", None)
>
> args = sys.argv[2:]
> --
> 1.7.4.1
>
This is the change I would like to have reviewed.
Sorry again for the spam.
A
^ permalink raw reply
* Re: [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:20 UTC (permalink / raw)
To: Andrei Warkentin; +Cc: git, gitster
In-Reply-To: <1318889798-17334-1-git-send-email-andreiw@vmware.com>
----- Original Message -----
> From: "Andrei Warkentin" <andreiw@vmware.com>
> To: git@vger.kernel.org, gitster@pobox.com
> Cc: "Andrei Warkentin" <andreiw@vmware.com>
> Sent: Monday, October 17, 2011 6:16:38 PM
> Subject: [PATCH] Git-p4: Add "git p4 change" command.
>
> Many users of p4/sd use changelists for review, regression
> tests and batch builds.
>
> "p4 change" is almost equivalent to "p4 submit", yet will
> just create the changelist and not submit it.
>
> Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
> ---
Sorry for the spam, this is the wrong patch :-(.
A
^ permalink raw reply
* [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:18 UTC (permalink / raw)
To: git, gitster; +Cc: Andrei Warkentin
Many users of p4/sd use changelists for review, regression
tests and batch builds.
"p4 change" is almost equivalent to "p4 submit", yet will
just create the changelist and not submit it.
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
contrib/fast-import/git-p4 | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..dd084b9 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -950,7 +950,10 @@ class P4Submit(Command, P4UserMap):
if checkModTime and (os.stat(fileName).st_mtime <= mtime):
response = "x"
while response != "y" and response != "n":
- response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+ if (self.cmdname == "change"):
+ response = raw_input("Change template unchanged. Create changelist anyway? [y]es, [n]o (skip this patch) ")
+ else:
+ response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
if response == "y":
tmpFile = open(fileName, "rb")
@@ -959,7 +962,10 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ if (self.cmdname == "change"):
+ p4_write_pipe("change -i", submitTemplate)
+ else:
+ p4_write_pipe("submit -i", submitTemplate)
if self.preserveUser:
if p4User:
@@ -981,9 +987,14 @@ class P4Submit(Command, P4UserMap):
file = open(fileName, "w+")
file.write(self.prepareLogMessage(template, logMessage))
file.close()
- print ("Perforce submit template written as %s. "
- + "Please review/edit and then use p4 submit -i < %s to submit directly!"
- % (fileName, fileName))
+ if (self.cmdname == "change"):
+ print ("Perforce change template written as %s. "
+ + "Please review/edit and then use p4 change -i < %s to submit directly!"
+ % (fileName, fileName))
+ else:
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 submit -i < %s to submit directly!"
+ % (fileName, fileName))
def run(self, args):
if len(args) == 0:
@@ -2177,6 +2188,7 @@ commands = {
"debug" : P4Debug,
"submit" : P4Submit,
"commit" : P4Submit,
+ "change" : P4Submit,
"sync" : P4Sync,
"rebase" : P4Rebase,
"clone" : P4Clone,
@@ -2202,6 +2214,7 @@ def main():
sys.exit(2)
options = cmd.options
+ cmd.cmdname = cmdName
cmd.gitdir = os.environ.get("GIT_DIR", None)
args = sys.argv[2:]
--
1.7.4.1
^ permalink raw reply related
* [PATCH] Git-p4: Add "git p4 change" command.
From: Andrei Warkentin @ 2011-10-17 22:16 UTC (permalink / raw)
To: git, gitster; +Cc: Andrei Warkentin
Many users of p4/sd use changelists for review, regression
tests and batch builds.
"p4 change" is almost equivalent to "p4 submit", yet will
just create the changelist and not submit it.
Signed-off-by: Andrei Warkentin <andreiw@vmware.com>
---
contrib/fast-import/git-p4 | 16 ++++++++++++----
contrib/fast-import/git-p4.txt | 10 ++++++++++
2 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2f7b270..19c295b 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
submitTemplate = message[:message.index(separatorLine)]
if self.isWindows:
submitTemplate = submitTemplate.replace("\r\n", "\n")
- p4_write_pipe("submit -i", submitTemplate)
+ if gitConfig("git-p4.changeOnSubmit"):
+ p4_write_pipe("change -i", submitTemplate)
+ else:
+ p4_write_pipe("subadasdmit -i", submitTemplate)
if self.preserveUser:
if p4User:
@@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
file = open(fileName, "w+")
file.write(self.prepareLogMessage(template, logMessage))
file.close()
- print ("Perforce submit template written as %s. "
- + "Please review/edit and then use p4 submit -i < %s to submit directly!"
- % (fileName, fileName))
+ if gitConfig("git-p4.changeOnSubmit"):
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 change -i < %s to create changelist!"
+ % (fileName, fileName))
+ else:
+ print ("Perforce submit template written as %s. "
+ + "Please review/edit and then use p4 submit -i < %s to submit directly!"
+ % (fileName, fileName))
def run(self, args):
if len(args) == 0:
diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
index 52003ae..3a3a815 100644
--- a/contrib/fast-import/git-p4.txt
+++ b/contrib/fast-import/git-p4.txt
@@ -180,6 +180,16 @@ git-p4.allowSubmit
git config [--global] git-p4.allowSubmit false
+git-p4.changeOnSubmit
+
+ git config [--global] git-p4.changeOnSubmit false
+
+Most places using p4/sourcedepot don't actually want you submit
+changes directly, and changelists are used to do regression testing,
+batch builds and review, hence, by setting this parameter to
+true you acknowledge you end up creating a changelist which you
+must then manually commit.
+
git-p4.syncFromOrigin
A useful setup may be that you have a periodically updated git repository
--
1.7.4.1
^ permalink raw reply related
* Re: [PATCH] resolve_gitlink_packed_ref(): fix mismerge
From: Mark Levedahl @ 2011-10-17 22:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git
In-Reply-To: <7vbotfmpbh.fsf_-_@alter.siamese.dyndns.org>
On 10/17/2011 02:43 PM, Junio C Hamano wrote:
> 2c5c66b (Merge branch 'jp/get-ref-dir-unsorted', 2011-10-10) merged a
> topic that forked from the mainline before a new helper function
> get_packed_refs() refactored code to read packed-refs file. The merge made
> the call to the helper function with an incorrect argument. The parameter
> to the function has to be a path to the submodule.
>
> Fix the mismerge.
>
> Helped-by: Mark Levedahl<mlevedahl@gmail.com>
> Helped-by: Michael Haggerty<mhagger@alum.mit.edu>
> Signed-off-by: Junio C Hamano<gitster@pobox.com>
> ---
>
Thank you, this fixes the problem for me.
Mark
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Jakub Narebski @ 2011-10-17 22:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kato Kazuyoshi, git
In-Reply-To: <7v62jnl3ef.fsf@alter.siamese.dyndns.org>
On Mon, 17 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>> By the way, it is common to either have following patches to be chain
>> reply to first patch, or better provide cover letter for patch series
>> and have all patches be reply to cover letter.
>
> It may be a good discipline for 14 patch series, but it doesn't matter for
> something this small.
Well, cover letter for 2-patch series might be overkill, but having
patches in series connected e.g. chain-replied-to, or all replies to
first patch in series, is IMHO a good idea.
>>> gitweb/gitweb.perl | 24 +++++++++++++-----------
>>> 1 files changed, 13 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index 85d64b2..095adda 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>>> # combined diff
>>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>>> if ($line =~ m/^\@{3}/) {
>>> - $diff_class = " chunk_header";
>>> + $diff_class = "chunk_header";
>>> } elsif ($line =~ m/^\\/) {
>>> - $diff_class = " incomplete";
>>> + $diff_class = "incomplete";
>>> } elsif ($prefix =~ tr/+/+/) {
>>> - $diff_class = " add";
>>> + $diff_class = "add";
>>> } elsif ($prefix =~ tr/-/-/) {
>>> - $diff_class = " rem";
>>> + $diff_class = "rem";
>>> }
>>
>> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
>> be hard to replace without given ... when, I think.
>
> I wasn't reading the existing context line, but now that you mention it,
> they are not just ugly but are borderline of being incorrect (e.g. it does
> not catch a broken hunk-header that begins with "@@@@" for a two-way
> merge).
>
> Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way
> merge), shouldn't the code be more like this?
>
> # combined diff
Wouldn't that cover not only combined diff, but an "ordinary" diff as
well then (i.e. comment should change)? I think that was the intent,
isn't it?
> my $num_sign = @{$from->{'href'}} + 1;
$from->{'href'} is array reference only for combined diff (>= 2 parents).
For ordinary diff it is a scalar.
> my @cc_classifier = (
> ["\@{$num_sign} ", "chunk_header"],
O.K.
Nb., it is "chunk_header", or "hunk_header"?
> ['\\', "incomplete"],
O.K.
> [" {$num_sign}", ""],
O.K.
> ["[+ ]{$num_sign}", "add"],
> ["[- ]{$num_sign}", "rem"],
It would be slightly different to what current code does.
Current code for combined diff uses "add" if there is at least one '+',
"rem" if there are no '+' and at least one '-', and context otherwise.
I wonder if with sufficiently evil merge we can have a line that is
added (changed) in some children, and removed in other, i.e. pluses
and minuses combined.
Nb. we can put regexp here, not only stringification of regexp.
i.e.
[qr/[+ ]{$num_sign}/, "add"],
[qr/[- ]{$num_sign}/, "rem"],
> );
> for my $cls (@cc_classifier) {
> if ($line =~ /^$cls->[0]$/) {
> $diff_class = $cls->[1];
> last;
> }
> }
Nice trick.
> Also don't we want to use "context" or something for the css class for the
> context lines, instead of assuming that we won't want to paint it in any
> special color?
Right. We use "diff" class without anything else for context, but probably
it would be better to state this explicitly.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: What should "git fetch origin +next" should do?
From: Marc Branchaud @ 2011-10-17 22:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vipnnmppx.fsf@alter.siamese.dyndns.org>
On 11-10-17 02:34 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I think the exact same confusion exists. I told git to update 'next'
>> from origin, but it didn't touch refs/remotes/origin/next.
>
> Except that you didn't tell git to *update* the remote tracking branch for
> 'next'; you merely told it to fetch 'next' at the remote.
>
>> ... But I suspect that is not how many git users think of it.
>
> I am inclined to agree that it might be the case; see my other message in
> this thread.
Indeed. Apologies for missing that subtlety.
So now I think option (2) is the best choice. To support one-off fetches,
teach fetch to accept "foo:" refspecs as "fetch ref foo from the remote and
only update FETCH_HEAD" -- maybe allow "foo:FETCH_HEAD" too, for folks who
like to be explicit and can't remember the shorthand syntax.
The rest of this post explains my reasoning, which I think pretty much just
rehashes what Junio said more efficiently in his initial message.
Overall I'd expect "git fetch origin next" to be a subset of "git fetch
origin". That is, since the default fetch refspec is
+refs/heads/*:refs/remotes/origin/*
normally "git fetch origin" gets all of origin's updated refs (ff or not) and
puts them under the local remotes/origin/ space. So I would expect "git
fetch origin next" to only fetch the "next" ref from origin and update the
local remotes/origin/next ref.
Given the default fetch refspec, I'd expect "git fetch origin +next" to do
the exact same thing. The + on the command line is basically redundant.
But removing the + from the fetch refspec changes things. Now I'd expect
plain "git fetch origin" to fail if there are any non-ff updates, and "git
fetch origin next" should also fail if origin's next ref is non-ff. But "git
fetch origin +next" would succeed.
In all cases if the command-line refspec has no RHS then git should try to
figure out which local ref to update from the config, and it should die if it
can't figure out a local ref to create or update. (As I said above, maybe
allow "git fetch origin foo:" to let the user put the tip of origin's foo ref
into FETCH_HEAD.)
All this gets a bit more complicated if the user has currently checked out
the a ref that should be updated (regardless of the presence of a LHS +).
But really only old-style git gurus should run in to that problem.
M.
^ permalink raw reply
* Re: [PATCH/RFC 3/2] Refactor Git::config_*
From: Jakub Narebski @ 2011-10-17 21:47 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <7vsjmrl4ur.fsf@alter.siamese.dyndns.org>
On Mon, 17 Oct 2011, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
> > This is version which has fixed style to be more Perl-ish, and which
> > actually works (i.e. t9700 passes).
> >
> > I have also moved _config_common() after commands that use it, just like
> > it is done with other "private" methods (methods with names starting with
> > '_'), and excluded this private detail of implementation from docs.
>
> It seems that this breaks many tests in t9xxx series for me, especially
> the 9100 series that cover git-svn.
Sorry about that, I have ran only t9700-perl-git.sh, which runs all right.
The problem was with duplicated _maybe_self(@_), which should be run only
once. t9700-perl-git.sh passed because it uses only (recommended) object
form, and not procedural form like git-svn.
The following amend fixes the issue for me:
-- >8 --
diff --git i/perl/Git.pm w/perl/Git.pm
index 8e52290..32f6533 100644
--- i/perl/Git.pm
+++ w/perl/Git.pm
@@ -653,7 +653,7 @@ sub config_int {
# Common subroutine to implement bulk of what the config* family of methods
# do. This wraps command('config') so it is not so fast.
sub _config_common {
- my ($self, $var, $opts) = _maybe_self(@_);
+ my ($self, $var, $opts) = @_;
try {
my @cmd = ('config', $opts->{'kind'} ? $opts->{'kind'} : ());
-- 8< --
I'll resend amended commit.
--
Jakub Narebski
Poland
^ permalink raw reply related
* Re: [PATCH] Allow to specify the editor used for git rebase -i by config/environment var
From: Junio C Hamano @ 2011-10-17 21:36 UTC (permalink / raw)
To: Peter Oberndorfer; +Cc: git, Phil Hord
In-Reply-To: <57346812.Rh0UlzroDp@soybean>
Peter Oberndorfer <kumbayo84@arcor.de> writes:
> The search order for choosing the sequence editor is:
> $GIT_SEQUENCE_EDITOR
> git config sequence.editor
> git var GIT_EDITOR (default editor for commit messages)
>
> With this change is it possible to have a separate
> (possibly graphical) editor that helps the user
> during a interactive rebase.
>
> Using $GIT_EDITOR or core.editor config var for this is not possible
> since they is also used to start the commit message editor for reword action.
Thanks. I'll reword the proposed commit log message before queuing,
because it won't make _any_ sense to talk about the search order before
telling what these things that are searched would do, or why they are
useful things to have.
"rebase -i": support special-purpose editor to edit insn sheet
The insn sheet used by "rebase -i" is designed to be easily editable by
any text editor, but an editor that is specifically meant for it (but
is otherwise unsuitable for editing regular text files) could be useful
by allowing drag & drop reordering in a GUI environment, for example.
The GIT_SEQUENCE_EDITOR environment variable and/or the sequence.editor
configuration variable can be used to specify such an editor, while
allowing the usual editor to be used to edit commit log messages. As
usual, the environment variable takes precedence over the configuration
variable.
It is envisioned that other "sequencer" based tools will use the same
mechanism.
Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
^ permalink raw reply
* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Jakub Narebski @ 2011-10-17 21:24 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> gitweb currently has a feature to show diff but it doesn't support
> "side-by-side" style. This modification introduces:
>
> * The "ds" query parameter to specify the style of diff.
> * The format_diff_chunk() to reorganize an output of diff.
> * The diff_nav() for form.
I would state it a bit differently.
I would say that this patch introduces support for side-by-side diff
in git_patchset_body, and that style of diff is controlled by newly
introduces 'diff_style' ("ds") parameter.
I would say a bit later that navigation bar got extended to make it
easy to switch between 'inline' and 'sidebyside' diff.
> ---
> gitweb/gitweb.perl | 81 +++++++++++++++++++++++++++++++++++++++++----
> gitweb/static/gitweb.css | 15 ++++++++
> 2 files changed, 88 insertions(+), 8 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 095adda..dca09dc 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -757,6 +757,7 @@ our @cgi_param_mapping = (
> extra_options => "opt",
> search_use_regexp => "sr",
> ctag => "by_tag",
> + diff_style => "ds",
> # this must be last entry (for manipulation from JavaScript)
> javascript => "js"
> );
O.K.
> @@ -1072,6 +1073,8 @@ sub evaluate_and_validate_params {
> }
> $search_regexp = $search_use_regexp ? $searchtext : quotemeta $searchtext;
> }
> +
> + $input_params{diff_style} ||= 'inline';
> }
O.K.
> # path to the current git repository
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
> - return "$div_open$line</div>\n";
> + return $diff_class, "$div_open$line</div>\n";
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
If you are changing behavior of format_diff_line, by having it return
<class>, <formatted line> line pair, I think you should document it,
and change the name of function.
All format_* functions in gitweb return a single string, so that
print format_*(...);
works.
> @@ -4828,8 +4831,32 @@ sub git_difftree_body {
> print "</table>\n";
> }
>
It would be good idea to add some comment about this subroutine,
e.g. what parameters it accepts. And perhaps how it works, as it is
not obvious.
> +sub format_diff_chunk {
> + my @chunk = @_;
> +
> + my $first_class = $chunk[0]->[0];
> + my @partial = map { $_->[1] } grep { $_->[0] eq $first_class } @chunk;
> +
> + if (scalar @partial < scalar @chunk) {
> + return join '', ("<div class='chunk'><div class='old'>",
> + @partial,
> + "</div>",
> + "<div class='new'>",
> + (map {
> + $_->[1];
> + } @chunk[scalar @partial..scalar @chunk-1]),
> + "</div></div>");
> + } else {
> + return join '', ("<div class='chunk'><div class='",
> + ($first_class eq 'add' ? 'new' : 'old'),
> + "'>",
> + @partial,
> + "</div></div>");
> + }
> +}
> +
> sub git_patchset_body {
> - my ($fd, $difftree, $hash, @hash_parents) = @_;
> + my ($fd, $is_inline, $difftree, $hash, @hash_parents) = @_;
Hmmm... shouldn't changing git_patchset_body signature (calling
convention) be mentioned in commit message?
> my ($hash_parent) = $hash_parents[0];
>
> my $is_combined = (@hash_parents > 1);
> @@ -4940,12 +4967,31 @@ sub git_patchset_body {
>
> # the patch itself
> LINE:
> + my @chunk;
> while ($patch_line = <$fd>) {
> chomp $patch_line;
>
> next PATCH if ($patch_line =~ m/^diff /);
>
> - print format_diff_line($patch_line, \%from, \%to);
> + my ($class, $line) = format_diff_line($patch_line, \%from, \%to);
> + if ($is_inline) {
> + print $line;
> + } elsif ($class eq 'add' || $class eq 'rem') {
> + push @chunk, [ $class, $line ];
> + } else {
> + if (@chunk) {
> + print format_diff_chunk(@chunk);
> + @chunk = ();
> + } elsif ($class eq 'chunk_header') {
> + print $line;
> + } else {
> + print '<div class="chunk"><div class="old">',
> + $line,
> + '</div><div class="new">',
> + $line,
> + '</div></div>';
> + }
> + }
I wonder why you have output of context lines in side-by-side diff in
git_patchset_body, instead of having it all in format_diff_chunk.
Unless by chunk you mean something different than "unified format hunk"
that is defined e.g. in (diff.info)"Detailed Unified".
> }
>
> } continue {
> @@ -7053,7 +7099,8 @@ sub git_blobdiff {
> if ($format eq 'html') {
> print "<div class=\"page_body\">\n";
>
> - git_patchset_body($fd, [ \%diffinfo ], $hash_base, $hash_parent_base);
> + git_patchset_body($fd, $input_params{diff_style} eq 'inline',
> + [ \%diffinfo ], $hash_base, $hash_parent_base);
> close $fd;
>
> print "</div>\n"; # class="page_body"
> @@ -7078,6 +7125,22 @@ sub git_blobdiff_plain {
> git_blobdiff('plain');
> }
>
> +sub diff_nav {
> + my ($style) = @_;
> +
> + my %pairs = (inline => 'inline', 'sidebyside' => 'side by side');
> + join '', ($cgi->start_form({ method => 'get' }),
> + $cgi->hidden('p'),
> + $cgi->hidden('a'),
> + $cgi->hidden('h'),
> + $cgi->hidden('hp'),
> + $cgi->hidden('hb'),
> + $cgi->hidden('hpb'),
> + $cgi->popup_menu('ds', [keys %pairs], $style, \%pairs),
> + $cgi->submit('change'),
> + $cgi->end_form);
> +}
Why do you feel the need for form to select between diff formats,
instead of links switching between them, like for interactive and
non-interactive blame?
> +
> sub git_commitdiff {
> my %params = @_;
> my $format = $params{-format} || 'html';
> @@ -7230,7 +7293,8 @@ sub git_commitdiff {
> my $ref = format_ref_marker($refs, $co{'id'});
>
> git_header_html(undef, $expires);
> - git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash, $formats_nav);
> + git_print_page_nav('commitdiff','', $hash,$co{'tree'},$hash,
> + $formats_nav . diff_nav($input_params{diff_style}));
> git_print_header_div('commit', esc_html($co{'title'}) . $ref, $hash);
> print "<div class=\"title_text\">\n" .
> "<table class=\"object_header\">\n";
> @@ -7284,7 +7348,8 @@ sub git_commitdiff {
> $use_parents ? @{$co{'parents'}} : $hash_parent);
> print "<br/>\n";
>
> - git_patchset_body($fd, \@difftree, $hash,
> + git_patchset_body($fd, $input_params{diff_style} eq 'inline',
> + \@difftree, $hash,
> $use_parents ? @{$co{'parents'}} : $hash_parent);
> close $fd;
> print "</div>\n"; # class="page_body"
Only commitdiff? What about blobdiff?
> diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
> index 7d88509..dc84db2 100644
> --- a/gitweb/static/gitweb.css
> +++ b/gitweb/static/gitweb.css
> @@ -618,6 +618,21 @@ div.remote {
> cursor: pointer;
> }
>
> +/* side-by-side diff */
> +div.chunk {
> + overflow: hidden;
???
> +}
> +
> +div.chunk div.old {
> + float: left;
> + width: 50%;
> + overflow: hidden;
> +}
> +
> +div.chunk div.new {
> + margin-left: 50%;
> + width: 50%;
> +}
Hmmm... I think the way side-by-side diff is styled should be
explained in commit message, or in comments.
I also wonder if it wouldn't be simpler to use table here, instead of
fiddling with floats, widths and margins.
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Junio C Hamano @ 2011-10-17 21:22 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Kato Kazuyoshi, git
In-Reply-To: <m38voj72xy.fsf@localhost.localdomain>
Jakub Narebski <jnareb@gmail.com> writes:
> By the way, it is common to either have following patches to be chain
> reply to first patch, or better provide cover letter for patch series
> and have all patches be reply to cover letter.
It may be a good discipline for 14 patch series, but it doesn't matter for
something this small.
>> gitweb/gitweb.perl | 24 +++++++++++++-----------
>> 1 files changed, 13 insertions(+), 11 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 85d64b2..095adda 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -2235,28 +2235,30 @@ sub format_diff_line {
>> # combined diff
>> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
>> if ($line =~ m/^\@{3}/) {
>> - $diff_class = " chunk_header";
>> + $diff_class = "chunk_header";
>> } elsif ($line =~ m/^\\/) {
>> - $diff_class = " incomplete";
>> + $diff_class = "incomplete";
>> } elsif ($prefix =~ tr/+/+/) {
>> - $diff_class = " add";
>> + $diff_class = "add";
>> } elsif ($prefix =~ tr/-/-/) {
>> - $diff_class = " rem";
>> + $diff_class = "rem";
>> }
>
> Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
> be hard to replace without given ... when, I think.
I wasn't reading the existing context line, but now that you mention it,
they are not just ugly but are borderline of being incorrect (e.g. it does
not catch a broken hunk-header that begins with "@@@@" for a two-way
merge).
Assuming that $from->{'href'} has all the parents (i.e. 2 for two-way
merge), shouldn't the code be more like this?
# combined diff
my $num_sign = @{$from->{'href'}} + 1;
my @cc_classifier = (
["\@{$num_sign} ", "chunk_header"],
['\\', "incomplete"],
[" {$num_sign}", ""],
["[+ ]{$num_sign}", "add"],
["[- ]{$num_sign}", "rem"],
);
for my $cls (@cc_classifier) {
if ($line =~ /^$cls->[0]$/) {
$diff_class = $cls->[1];
last;
}
}
Also don't we want to use "context" or something for the css class for the
context lines, instead of assuming that we won't want to paint it in any
special color?
^ permalink raw reply
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Junio C Hamano @ 2011-10-17 21:03 UTC (permalink / raw)
To: Jeff King; +Cc: Clemens Buchacher, git
In-Reply-To: <20111017020912.GB18536@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
>
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".
I personally think this is going a bit too far, even for "informative"
option, by allowing to fish for possible list of usernames. It would make
it a tougher sell to later default to "informative", I am afraid.
Suppose you are setting up your own repository (either on your own box or
on a box with a separate administrator), and youare wondering why your
attempted access failed. You know /pub/repo/sito.git exists (you created
it after all) and you get "no such repository: /repo/sito.git" when you
ran:
$ git clone git://host/repo/sito.git/
If you have another repository in /pub/repo/ that does already work, and
if you know /pub/repo/sito.git/ is fine locally (e.g. you can see local
command like "git log" works fine there), then even if you see "not found"
you would know to compare what the difference between these two are.
If there is no other repositories in /pub/repo/ or you are setting up many
repositories on this same box for the first time, wouldn't it be plausible
that you _are_ the administrator of the box and have access to the daemon
log to diagnose the problem more easily anyway?
I can see how this is "leaking a little more information", but I am not
convinced that leak is helping legit users more than helping unwanted
snoopers.
> The new calling conventions for this function seem a little weird. I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.
>
> One of:
>
> errno = EACCESS;
> return -1;
>
> or
>
> return -EACCESS;
>
> would be more idiomatic, I think.
Yes, the former would probably be easier to handle.
^ permalink raw reply
* Re: [PATCH] use test number as port number
From: Junio C Hamano @ 2011-10-17 20:57 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: Jeff King, git, Junio C Hamano
In-Reply-To: <20111017195547.GB29479@ecki>
Clemens Buchacher <drizzd@aon.at> writes:
> Test 5550 was apparently using the default port number by mistake.
>
> Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> ---
>
> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>>
>> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
>
> Thanks, I missed that.
>
> Clemens
>
> t/t5550-http-fetch.sh | 2 +-
> t/t5570-git-daemon.sh | 1 +
> 2 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
> index a1883ca..8a77750 100755
> --- a/t/t5550-http-fetch.sh
> +++ b/t/t5550-http-fetch.sh
> @@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
> test_done
> fi
>
> -. "$TEST_DIRECTORY"/lib-httpd.sh
> LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
> +. "$TEST_DIRECTORY"/lib-httpd.sh
> start_httpd
Good eyes. This is the only one in the 55xx series that gets the order
wrong.
I'll drop the patch to 5570 for now as that should be done in the change
that is still not in 'next' that adds 5570.
I've fixed and queued the previous one as aa0b028 (daemon: add tests,
2011-10-17); does that look good enough?
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index e6482eb..a92d996 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -3,6 +3,7 @@
> test_description='test fetching over git protocol'
> . ./test-lib.sh
>
> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
> . "$TEST_DIRECTORY"/lib-daemon.sh
> start_daemon
^ permalink raw reply
* Re: [PATCH/RFC 1/2] gitweb: change format_diff_line() to remove leading SP from $diff_class
From: Jakub Narebski @ 2011-10-17 20:56 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git, Jakub Narebski
In-Reply-To: <CAFo4x0LP4fXgSNAnss_WRLo-TH_qe=esYn7P+=iS6t87tdzcbw@mail.gmail.com>
By the way, it is common to either have following patches to be chain
reply to first patch, or better provide cover letter for patch series
and have all patches be reply to cover letter.
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
This commit needs some more explanation in the commit message, like
Junio said.
> The format_diff_line() will return $diff_class and HTML in upcoming changes.
This is not necessary, I think; this change stands alone as a style
(semantic) cleanup.
Signoff? See Documentation/SubmittingPatches
> ---
> gitweb/gitweb.perl | 24 +++++++++++++-----------
> 1 files changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 85d64b2..095adda 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -2235,28 +2235,30 @@ sub format_diff_line {
> # combined diff
> my $prefix = substr($line, 0, scalar @{$from->{'href'}});
> if ($line =~ m/^\@{3}/) {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($line =~ m/^\\/) {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> } elsif ($prefix =~ tr/+/+/) {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($prefix =~ tr/-/-/) {
> - $diff_class = " rem";
> + $diff_class = "rem";
> }
Hmmm... that reminds me: this if-elsif chain is a bit ugly, but would
be hard to replace without given ... when, I think.
> } else {
> # assume ordinary diff
> my $char = substr($line, 0, 1);
> if ($char eq '+') {
> - $diff_class = " add";
> + $diff_class = "add";
> } elsif ($char eq '-') {
> - $diff_class = " rem";
> + $diff_class = "rem";
> } elsif ($char eq '@') {
> - $diff_class = " chunk_header";
> + $diff_class = "chunk_header";
> } elsif ($char eq "\\") {
> - $diff_class = " incomplete";
> + $diff_class = "incomplete";
> }
This is also ugly, but this one could in theory be replaced by hash
lookup. But this would remove symmetry...
> }
> $line = untabify($line);
> +
> + my $div_open = '<div class="' . (join ' ', ('diff', $diff_class)) . '">';
Another solution would be to use
+
+ my $diff_classes = "diff";
+ $diff_classes .= " $diff_class" if ($diff_class);
+
> if ($from && $to && $line =~ m/^\@{2} /) {
> my ($from_text, $from_start, $from_lines, $to_text, $to_start,
> $to_lines, $section) =
> $line =~ m/^\@{2} (-(\d+)(?:,(\d+))?) (\+(\d+)(?:,(\d+))?) \@{2}(.*)$/;
> @@ -2274,7 +2276,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
The above is word-wrapped. Please turn off word wrapping in the
future to avoid such damage to the patch.
> - return "<div class=\"diff$diff_class\">$line</div>\n";
> + return "$div_open$line</div>\n";
This would be instead
+ return "<div class=\"$diff_classes\">$line</div>\n";
which is a bit more symmetric.
But that is just a matter of taste.
> } elsif ($from && $to && $line =~ m/^\@{3}/) {
> my ($prefix, $ranges, $section) = $line =~ m/^(\@+) (.*?) \@+(.*)$/;
> my (@from_text, @from_start, @from_nlines, $to_text, $to_start, $to_nlines);
> @@ -2307,9 +2309,9 @@ sub format_diff_line {
> }
> $line .= " $prefix</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1)
> . "</span>";
> - return "<div class=\"diff$diff_class\">$line</div>\n";
> + return "$div_open$line</div>\n";
> }
> - return "<div class=\"diff$diff_class\">" . esc_html($line, -nbsp=>1)
> . "</div>\n";
> + return $div_open . esc_html($line, -nbsp=>1) . "</div>\n";
> }
>
> # Generates undef or something like "_snapshot_" or "snapshot (_tbz2_ _zip_)",
> --
> 1.7.7.213.g8b0e1.dirty
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH/RFC 3/2] Refactor Git::config_*
From: Junio C Hamano @ 2011-10-17 20:50 UTC (permalink / raw)
To: Jakub Narebski; +Cc: Eric Wong, Cord Seele, Matthieu Moy, git, Cord Seele
In-Reply-To: <201110072317.17436.jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> This is version which has fixed style to be more Perl-ish, and which
> actually works (i.e. t9700 passes).
>
> I have also moved _config_common() after commands that use it, just like
> it is done with other "private" methods (methods with names starting with
> '_'), and excluded this private detail of implementation from docs.
It seems that this breaks many tests in t9xxx series for me, especially
the 9100 series that cover git-svn.
^ permalink raw reply
* [PATCH] Allow to specify the editor used for git rebase -i by config/environment var
From: Peter Oberndorfer @ 2011-10-17 20:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Phil Hord
The search order for choosing the sequence editor is:
$GIT_SEQUENCE_EDITOR
git config sequence.editor
git var GIT_EDITOR (default editor for commit messages)
With this change is it possible to have a separate
(possibly graphical) editor that helps the user
during a interactive rebase.
Using $GIT_EDITOR or core.editor config var for this is not possible
since they is also used to start the commit message editor for reword action.
Signed-off-by: Peter Oberndorfer <kumbayo84@arcor.de>
---
I reworded the commit message and the config description a bit.
renamed to sequence.editor / $GIT_SEQUENCE_EDITOR
and moved the helper to git-rebase--interactive.sh
Documentation/config.txt | 7 +++++++
git-rebase--interactive.sh | 15 ++++++++++++++-
2 files changed, 21 insertions(+), 1 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 03296b7..048c5f9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -473,6 +473,13 @@ core.editor::
variable when it is set, and the environment variable
`GIT_EDITOR` is not set. See linkgit:git-var[1].
+sequence.editor::
+ Text editor used by git rebase -i for editing the rebase todo file.
+ The value is meant to be interpreted by the shell when it is used.
+ It can be overridden by the 'GIT_SEQUENCE_EDITOR' environment variable.
+ When not configured the default commit message editor is used instead.
+ See linkgit:git-var[1]
+
core.pager::
The command that git will use to paginate output. Can
be overridden with the `GIT_PAGER` environment
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 94f36c2..13a0661 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -161,6 +161,19 @@ do_with_author () {
)
}
+git_sequence_editor() {
+ if test -z "${GIT_SEQUENCE_EDITOR:+set}"
+ then
+ GIT_SEQUENCE_EDITOR="$(git config sequence.editor)"
+ if [ -z "$GIT_SEQUENCE_EDITOR" ]
+ then
+ GIT_SEQUENCE_EDITOR="$(git var GIT_EDITOR)" || return $?
+ fi
+ fi
+
+ eval "$GIT_SEQUENCE_EDITOR" '"$@"'
+}
+
pick_one () {
ff=--ff
case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
@@ -832,7 +845,7 @@ has_action "$todo" ||
die_abort "Nothing to do"
cp "$todo" "$todo".backup
-git_editor "$todo" ||
+git_sequence_editor "$todo" ||
die_abort "Could not execute editor"
has_action "$todo" ||
--
1.7.7.329.g2140c
^ permalink raw reply related
* Re: [PATCH 1/2] daemon: add tests
From: Jeff King @ 2011-10-17 20:08 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Junio C Hamano
In-Reply-To: <20111017200528.GA19054@ecki>
On Mon, Oct 17, 2011 at 10:05:28PM +0200, Clemens Buchacher wrote:
> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
> >
> > Thanks, it's nice to have some tests. Overall, some of the tests feel a
> > little silly, because the results should be exactly the same as fetching
> > or pushing a local repository (so the "set-head" thing, for example,
> > really has little to do with git-daemon).
>
> Hmm, yes. Actually, I thought I had found a bug with the failure of
> "set-head -a". But now I see that in t5505 this treated like a
> feature.
It's not a feature, exactly. It's just documenting that we fail in the
face of ambiguous HEADs. Arguably, the test should be switched to use
text_expect_failure to document that we would prefer it the other way,
but it doesn't work now.
> Would it be difficult to support this over the git protocol? Maybe
> I will have a look.
It needs a protocol extension to communicate symbolic ref destinations.
The topic has come up a few times, and I think Junio even had patches at
one point.
-Peff
^ permalink raw reply
* Re: [PATCH 1/2] daemon: add tests
From: Clemens Buchacher @ 2011-10-17 20:05 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020103.GA18536@sigill.intra.peff.net>
On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>
> Thanks, it's nice to have some tests. Overall, some of the tests feel a
> little silly, because the results should be exactly the same as fetching
> or pushing a local repository (so the "set-head" thing, for example,
> really has little to do with git-daemon).
Hmm, yes. Actually, I thought I had found a bug with the failure of
"set-head -a". But now I see that in t5505 this treated like a
feature.
Would it be difficult to support this over the git protocol? Maybe
I will have a look.
Clemens
^ permalink raw reply
* [PATCH v2 2/2] daemon: report permission denied error to clients
From: Clemens Buchacher @ 2011-10-17 19:58 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <1318803076-4229-2-git-send-email-drizzd@aon.at>
If passed an inaccessible url, git daemon returns the
following error:
$ git clone git://host/repo
fatal: remote error: no such repository: /repo
In case of a permission denied error, return the following
instead:
fatal: remote error: permission denied: /repo
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Compared to v1 of this patch, the calling convention of path_ok are
back to what they were previously. Now the only change is that it
sets errno.
daemon.c | 15 +++++++++++++--
path.c | 31 +++++++++++++++++++++----------
t/t5570-git-daemon.sh | 2 +-
3 files changed, 35 insertions(+), 13 deletions(-)
diff --git a/daemon.c b/daemon.c
index 72fb53a..2f7f84e 100644
--- a/daemon.c
+++ b/daemon.c
@@ -120,12 +120,14 @@ static char *path_ok(char *directory)
if (daemon_avoid_alias(dir)) {
logerror("'%s': aliased", dir);
+ errno = 0;
return NULL;
}
if (*dir == '~') {
if (!user_path) {
logerror("'%s': User-path not allowed", dir);
+ errno = EACCES;
return NULL;
}
if (*user_path) {
@@ -158,6 +160,7 @@ static char *path_ok(char *directory)
if (*dir != '/') {
/* Allow only absolute */
logerror("'%s': Non-absolute path denied (interpolated-path active)", dir);
+ errno = EACCES;
return NULL;
}
@@ -173,6 +176,7 @@ static char *path_ok(char *directory)
if (*dir != '/') {
/* Allow only absolute */
logerror("'%s': Non-absolute path denied (base-path active)", dir);
+ errno = EACCES;
return NULL;
}
snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
@@ -190,7 +194,9 @@ static char *path_ok(char *directory)
}
if (!path) {
+ int err = errno;
logerror("'%s' does not appear to be a git repository", dir);
+ errno = err;
return NULL;
}
@@ -221,6 +227,7 @@ static char *path_ok(char *directory)
}
logerror("'%s': not in whitelist", path);
+ errno = EACCES;
return NULL; /* Fallthrough. Deny by default */
}
@@ -269,8 +276,12 @@ static int run_service(char *dir, struct daemon_service *service)
return daemon_error(dir, "service not enabled");
}
- if (!(path = path_ok(dir)))
- return daemon_error(dir, "no such repository");
+ if (!(path = path_ok(dir))) {
+ if (errno == EACCES)
+ return daemon_error(dir, "permission denied");
+ else
+ return daemon_error(dir, "no such repository");
+ }
/*
* Security on the cheap.
diff --git a/path.c b/path.c
index 6f3f5d5..227d8d7 100644
--- a/path.c
+++ b/path.c
@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)
static char used_path[PATH_MAX];
static char validated_path[PATH_MAX];
+ errno = 0;
if (!path)
return NULL;
@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)
path[len-1] = 0;
len--;
}
- if (PATH_MAX <= len)
+ if (PATH_MAX <= len) {
+ errno = ENAMETOOLONG;
return NULL;
+ }
if (path[0] == '~') {
char *newpath = expand_user_path(path);
if (!newpath || (PATH_MAX - 10 < strlen(newpath))) {
free(newpath);
+ errno = 0;
return NULL;
}
/*
@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)
strcpy(validated_path, path);
path = used_path;
}
- else if (PATH_MAX - 10 < len)
+ else if (PATH_MAX - 10 < len) {
+ errno = ENAMETOOLONG;
return NULL;
- else {
+ } else {
path = strcpy(used_path, path);
strcpy(validated_path, path);
}
@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)
if (!access(path, F_OK)) {
strcat(validated_path, suffix[i]);
break;
+ } else if (errno == EACCES) {
+ return NULL;
}
}
- if (!suffix[i] || chdir(path))
+ if (!suffix[i])
+ return NULL;
+ if (chdir(path))
return NULL;
path = validated_path;
}
else if (chdir(path))
return NULL;
- if (access("objects", X_OK) == 0 && access("refs", X_OK) == 0 &&
- validate_headref("HEAD") == 0) {
- set_git_dir(".");
- check_repository_format();
- return path;
+ if (access("objects", X_OK) || access("refs", X_OK))
+ return NULL;
+ if (validate_headref("HEAD")) {
+ errno = 0;
+ return NULL;
}
- return NULL;
+ set_git_dir(".");
+ check_repository_format();
+ return path;
}
int set_shared_perm(const char *path, int mode)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index aa5771a..e6482eb 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -141,7 +141,7 @@ start_daemon --informative-errors
test_expect_success 'clone non-existent' "test_remote_error clone nowhere.git 'no such repository'"
test_expect_success 'push disabled' "test_remote_error push repo.git 'service not enabled'"
-test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'no such repository'"
+test_expect_success 'read access denied' "test_remote_error -x fetch repo.git 'permission denied'"
test_expect_success 'not exported' "test_remote_error -n fetch repo.git 'repository not exported'"
stop_daemon
--
1.7.7
^ permalink raw reply related
* [PATCH] use test number as port number
From: Clemens Buchacher @ 2011-10-17 19:55 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020103.GA18536@sigill.intra.peff.net>
Test 5550 was apparently using the default port number by mistake.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:
>
> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
Thanks, I missed that.
Clemens
t/t5550-http-fetch.sh | 2 +-
t/t5570-git-daemon.sh | 1 +
2 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh
index a1883ca..8a77750 100755
--- a/t/t5550-http-fetch.sh
+++ b/t/t5550-http-fetch.sh
@@ -8,8 +8,8 @@ if test -n "$NO_CURL"; then
test_done
fi
-. "$TEST_DIRECTORY"/lib-httpd.sh
LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}
+. "$TEST_DIRECTORY"/lib-httpd.sh
start_httpd
test_expect_success 'setup repository' '
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index e6482eb..a92d996 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -3,6 +3,7 @@
test_description='test fetching over git protocol'
. ./test-lib.sh
+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}
. "$TEST_DIRECTORY"/lib-daemon.sh
start_daemon
--
1.7.7
^ permalink raw reply related
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Jeff King @ 2011-10-17 19:51 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Junio C Hamano
In-Reply-To: <20111017194821.GA29479@ecki>
On Mon, Oct 17, 2011 at 09:48:21PM +0200, Clemens Buchacher wrote:
> > I like the intent. This actually does leak a little more information
> > than the existing --informative-errors, as before you couldn't tell the
> > difference between "not found" and "not exported".
>
> I think you mean that before, you couldn't tell the difference
> between "not found" and "permission denied".
Ah, right. Sorry, I was thinking path_ok handled the export-ok flag, but
I already handled it in my patch to run_service. So it is leaking a
little more, but even less than I indicated. And at any rate, I think it
is consistent with what --informative-errors is meant to do, so it's a
good change.
> > The new calling conventions for this function seem a little weird. I
> > would expect either "return negative, and set errno" for usual library
> > code, or possibly "return negative error value". But "return -1, or a
> > positive error code" seems unusual to me.
>
> Yes indeed, will fix.
Thanks.
-Peff
^ permalink raw reply
* Re: [PATCH 2/2] daemon: report permission denied error to clients
From: Clemens Buchacher @ 2011-10-17 19:48 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20111017020912.GB18536@sigill.intra.peff.net>
On Sun, Oct 16, 2011 at 10:09:12PM -0400, Jeff King wrote:
> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:
>
> > If passed an inaccessible url, git daemon returns the
> > following error:
> >
> > $ git clone git://host/repo
> > fatal: remote error: no such repository: /repo
> >
> > In case of a permission denied error, return the following
> > instead:
> >
> > fatal: remote error: permission denied: /repo
> >
> > Signed-off-by: Clemens Buchacher <drizzd@aon.at>
> > ---
>
> I like the intent. This actually does leak a little more information
> than the existing --informative-errors, as before you couldn't tell the
> difference between "not found" and "not exported".
I think you mean that before, you couldn't tell the difference
between "not found" and "permission denied".
> > -static char *path_ok(char *directory)
> > +static int path_ok(char *directory, const char **return_path)
> > {
> > static char rpath[PATH_MAX];
> > static char interp_path[PATH_MAX];
> > @@ -120,13 +120,13 @@ static char *path_ok(char *directory)
> >
> > if (daemon_avoid_alias(dir)) {
> > logerror("'%s': aliased", dir);
> > - return NULL;
> > + return -1;
> > }
> >
> > if (*dir == '~') {
> > if (!user_path) {
> > logerror("'%s': User-path not allowed", dir);
> > - return NULL;
> > + return EACCES;
>
> The new calling conventions for this function seem a little weird. I
> would expect either "return negative, and set errno" for usual library
> code, or possibly "return negative error value". But "return -1, or a
> positive error code" seems unusual to me.
Yes indeed, will fix.
Clemens
^ permalink raw reply
* Re: [PATCH v4 0/7] Provide API to invalidate refs cache
From: Michael Haggerty @ 2011-10-17 19:43 UTC (permalink / raw)
To: Heiko Voigt
Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
Johan Herland, Julian Phillips
In-Reply-To: <20111017184708.GB2540@sandbox-rc>
On 10/17/2011 08:47 PM, Heiko Voigt wrote:
> On Mon, Oct 17, 2011 at 04:38:04AM +0200, mhagger@alum.mit.edu wrote:
>> [1] http://marc.info/?l=git&m=131827641227965&w=2
>> In this mailing list thread, Heiko Voigt stated that git-submodule
>> does not modify any references, so it should not have to use the
>> API.
>
> This is not entirely true. I was saying that my submodule-merge code is
> currently the only one using the refs api for submodules and that does
> not need to modify submodule refs. I imagine that there will be some
> users when submodule support matures (e.g. recursive push).
Sorry for misunderstanding/misrepresenting you; thanks for the
clarification.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 2/2] gitweb: add a feature to show side-by-side diff
From: Junio C Hamano @ 2011-10-17 19:37 UTC (permalink / raw)
To: Kato Kazuyoshi; +Cc: git
In-Reply-To: <CAFo4x0L4BAKnCDa1uEK0Rskd9kTsR-94D4mkYKnLGqVDnuyuBA@mail.gmail.com>
Kato Kazuyoshi <kato.kazuyoshi@gmail.com> writes:
> @@ -2276,7 +2279,7 @@ sub format_diff_line {
> }
> $line = "<span class=\"chunk_info\">@@ $from_text $to_text @@</span>" .
> "<span class=\"section\">" . esc_html($section, -nbsp=>1) .
> "</span>";
Corrupt patch (wrapped long line); please fix.
I suspect you have similar issues in the [PATCH 1/2] message, too.
^ 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