* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Johannes Schindelin @ 2012-02-23 22:37 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <CABPQNSZ+=no5PKkaa8_44FbOzh_W4A5knoxKgDQ2wTvOaeKdew@mail.gmail.com>
Hi kusma,
On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
> On Thu, Feb 23, 2012 at 10:26 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
> >
> >> Our stat implementation for Windows always sets st_ino to 0. This
> >> means that checking if isatty(0) and comparing the reported inodes
> >> of stdout and stdin is not sufficient to detect that both are
> >> pointing to the same TTY.
> >>
> >> Luckily, there's only one console on Windows, so adding a check for
> >> isatty(1) should do the trick. For platforms where inodes are
> >> reported correctly, this should still be correct.
> >
> > Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
> > not? Should they not access the very same console object?
> >
>
> The point is that they might not. The old test would draw the wrong
> conclusion if isatty(0) was true, but isatty(1) was not because it
> used st_ino to verify that stdin and stdout pointed to the same
> terminal. The reason it did that was to catch cases where stdin and
> stdout pointed to different terminals, AFAICT. Not checking isatty(1)
> was simply an optimization, which works when st_ino is filled out
> correctly.
>
> On Windows there is only one terminal, so it's sufficient to check if
> both are connected.
Ah, okay. Thanks for answering so patiently!
ACK.
Ciao,
Dscho
^ permalink raw reply
* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Erik Faye-Lund @ 2012-02-23 21:36 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <alpine.DEB.1.00.1202231525320.3340@s15462909.onlinehome-server.info>
On Thu, Feb 23, 2012 at 10:26 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
>
>> Our stat implementation for Windows always sets st_ino to 0. This
>> means that checking if isatty(0) and comparing the reported inodes
>> of stdout and stdin is not sufficient to detect that both are
>> pointing to the same TTY.
>>
>> Luckily, there's only one console on Windows, so adding a check for
>> isatty(1) should do the trick. For platforms where inodes are
>> reported correctly, this should still be correct.
>
> Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
> not? Should they not access the very same console object?
>
The point is that they might not. The old test would draw the wrong
conclusion if isatty(0) was true, but isatty(1) was not because it
used st_ino to verify that stdin and stdout pointed to the same
terminal. The reason it did that was to catch cases where stdin and
stdout pointed to different terminals, AFAICT. Not checking isatty(1)
was simply an optimization, which works when st_ino is filled out
correctly.
On Windows there is only one terminal, so it's sufficient to check if
both are connected.
^ permalink raw reply
* Re: [msysGit] [PATCH] mingw: work around stat-limitation
From: Johannes Schindelin @ 2012-02-23 21:26 UTC (permalink / raw)
To: Erik Faye-Lund; +Cc: git, gitster, j6t, msysgit
In-Reply-To: <1330028744-5280-1-git-send-email-kusmabite@gmail.com>
Hi,
On Thu, 23 Feb 2012, Erik Faye-Lund wrote:
> Our stat implementation for Windows always sets st_ino to 0. This
> means that checking if isatty(0) and comparing the reported inodes
> of stdout and stdin is not sufficient to detect that both are
> pointing to the same TTY.
>
> Luckily, there's only one console on Windows, so adding a check for
> isatty(1) should do the trick. For platforms where inodes are
> reported correctly, this should still be correct.
Sorry to ask so stupidly, but why does isatty(1) work and isatty(0) does
not? Should they not access the very same console object?
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCHv3 0/3] gitweb: Faster project search
From: Junio C Hamano @ 2012-02-23 20:54 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <1330011779-7803-1-git-send-email-jnareb@gmail.com>
Jakub Narebski <jnareb@gmail.com> writes:
> [Cc-ing Junio because of his involvement in discussion]
Heh, drop that line. People can guess that from the context.
> These patches are separated from first part of previous version of
> this series
>
> "[PATCHv2 0/8] gitweb: Faster and improved project search"
> http://thread.gmane.org/gmane.comp.version-control.git/190852
>
> It is meant to replace 'jn/gitweb-search-optim' in pu
I think the result is a lot easier than the previous rounds; will replace
the said topic and move it to 'next' soonish.
Thanks.
^ permalink raw reply
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Erik Faye-Lund @ 2012-02-23 20:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlint2t5h.fsf@alter.siamese.dyndns.org>
On Thu, Feb 23, 2012 at 9:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>>> + /* Use editor if stdin and stdout are the same and is a tty */
>>>> + return (!fstat(0, &st_stdin) &&
>>>> + !fstat(1, &st_stdout) &&
>>>> + isatty(0) &&
>>>> + st_stdin.st_dev == st_stdout.st_dev &&
>>>> + st_stdin.st_ino == st_stdout.st_ino &&
>>>> + st_stdin.st_mode == st_stdout.st_mode);
>>>
>>> I just stumbled over this code, and I got a bit worried; the
>>> stat-implementation we use on Windows sets st_ino to 0, so
>>> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>>>
>>> Perhaps we should add a check for isatty(1) here as well? ...
>>> Or is there something I'm missing here?
>>
>> No, the intention was ...
>
> s/No,/No, you are not missing anything./;
>
> I'll queue it with this explanation:
>
> merge: do not trust fstat(2) too much when checking interactiveness
>
> The heuristic used by "git merge" to decide if it automatically gives an
> editor upon clean automerge is to see if the standard input and the
> standard output is the same device and is a tty, we are in an interactive
> session. "The same device" test was done by comparing fstat(2) result on
> the two file descriptors (and they must match), and we asked isatty() only
> for the standard input (we insist that they are the same device and there
> is no point asking tty-ness of the standard output).
>
> The stat(2) emulation on Windows port however does not give a usable value
> in st_ino field, so even if the standard output is connected to something
Shouldn't that be "emulation _in the_ Windows port" and "in _the_ st_ino field"?
> different from the standard input, "The same device" test may incorrectly
> return true. To accomodate it, add another isatty() check for the standard
> output stream as well.
>
> Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Thanks.
I just sent a mail with a proper-ish commit message, but I like yours
better as it explains the symptom a bit.
^ permalink raw reply
* Re: [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Junio C Hamano @ 2012-02-23 20:28 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: git
In-Reply-To: <1330013115-26355-1-git-send-email-cmn@elego.de>
Carlos Martín Nieto <cmn@elego.de> writes:
> The current message is too long and at too low a level for anybody to
> understand it if they don't know about the configuration format
> already.
>
> Reformat it to show the commands a user would be expected to use,
> instead of the contents of the configuration file.
> ---
Sounds like a change going in the right direction. I am unsure if it is a
good idea to remove "See git-config...", but otherwise I like the updated
text much better.
But of course I am not the target audience, so let's see what we hear from
others.
Thanks.
^ permalink raw reply
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Junio C Hamano @ 2012-02-23 20:26 UTC (permalink / raw)
To: kusmabite; +Cc: git
In-Reply-To: <7vd3954ame.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> + /* Use editor if stdin and stdout are the same and is a tty */
>>> + return (!fstat(0, &st_stdin) &&
>>> + !fstat(1, &st_stdout) &&
>>> + isatty(0) &&
>>> + st_stdin.st_dev == st_stdout.st_dev &&
>>> + st_stdin.st_ino == st_stdout.st_ino &&
>>> + st_stdin.st_mode == st_stdout.st_mode);
>>
>> I just stumbled over this code, and I got a bit worried; the
>> stat-implementation we use on Windows sets st_ino to 0, so
>> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>>
>> Perhaps we should add a check for isatty(1) here as well? ...
>> Or is there something I'm missing here?
>
> No, the intention was ...
s/No,/No, you are not missing anything./;
I'll queue it with this explanation:
merge: do not trust fstat(2) too much when checking interactiveness
The heuristic used by "git merge" to decide if it automatically gives an
editor upon clean automerge is to see if the standard input and the
standard output is the same device and is a tty, we are in an interactive
session. "The same device" test was done by comparing fstat(2) result on
the two file descriptors (and they must match), and we asked isatty() only
for the standard input (we insist that they are the same device and there
is no point asking tty-ness of the standard output).
The stat(2) emulation on Windows port however does not give a usable value
in st_ino field, so even if the standard output is connected to something
different from the standard input, "The same device" test may incorrectly
return true. To accomodate it, add another isatty() check for the standard
output stream as well.
Reported-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Thanks.
^ permalink raw reply
* [PATCH] mingw: work around stat-limitation
From: Erik Faye-Lund @ 2012-02-23 20:25 UTC (permalink / raw)
To: git; +Cc: gitster, j6t, msysgit
Our stat implementation for Windows always sets st_ino to 0. This
means that checking if isatty(0) and comparing the reported inodes
of stdout and stdin is not sufficient to detect that both are
pointing to the same TTY.
Luckily, there's only one console on Windows, so adding a check for
isatty(1) should do the trick. For platforms where inodes are
reported correctly, this should still be correct.
Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
Here's a proper patch for this glitch.
builtin/merge.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index ed0f959..bef01e3 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1130,6 +1130,7 @@ static int default_edit_option(void)
return (!fstat(0, &st_stdin) &&
!fstat(1, &st_stdout) &&
isatty(0) &&
+ isatty(1) &&
st_stdin.st_dev == st_stdout.st_dev &&
st_stdin.st_ino == st_stdout.st_ino &&
st_stdin.st_mode == st_stdout.st_mode);
--
1.7.9
^ permalink raw reply related
* Re: measuring the % change between two commits
From: Junio C Hamano @ 2012-02-23 20:13 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gXQaE+RZxe4S7vqAyPka9N9PAoe+557FaLO+JJoJFshA@mail.gmail.com>
Sitaram Chamarty <sitaramc@gmail.com> writes:
> I could do a --numstat and then do a 'wc -l' on each file I guess, but
> I was hoping to avoid that.
>
> --dirstat gives you a percentage but does not count the top level directory.
Note that dirstat is not about "how much damage was caused to the entire
codebase". It only measures "How is the damage this patch causes
distributed across directories it touches". It was unclear from your "a %
measure for the changes between two commits" which one you meant, but I am
guessing from your "--numstat and wc -l" reference that you are asking for
the former, e.g. we have 300,000 lines of code and between these two
commits 10,000 lines changed, hence we updated 3% of the codebase during
that period".
^ permalink raw reply
* Re: measuring the % change between two commits
From: Junio C Hamano @ 2012-02-23 20:09 UTC (permalink / raw)
To: Sitaram Chamarty; +Cc: Git Mailing List
In-Reply-To: <CAMK1S_gXQaE+RZxe4S7vqAyPka9N9PAoe+557FaLO+JJoJFshA@mail.gmail.com>
Sitaram Chamarty <sitaramc@gmail.com> writes:
> I could do a --numstat and then do a 'wc -l' on each file I guess, but
> I was hoping to avoid that.
Either you do it or the git core does it, but I am not sure if the use
case is common enough to warrant additional code on the core side.
^ permalink raw reply
* Re: git log -z doesn't separate commits with NULs
From: Junio C Hamano @ 2012-02-23 20:07 UTC (permalink / raw)
To: Jeff King; +Cc: Nikolaj Shurkaev, Jakub Narebski, git
In-Reply-To: <20120223193451.GB30132@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
>
> Doesn't:
>
> git format-patch HEAD~3..HEAD SomePath
>
> do what you want? It is certainly designed to, and it seems to work for
> me.
It is not quite "designed to", though.
It happens to work that way, and I do not think we want to forbid its use,
but we would want to discourage anybody from blindly using it without
thinking if the end results suits his/her purpose (and the reason should
be obvious to those who think, the hint is "log message").
^ permalink raw reply
* Re: [PATCHv5] git-p4: add initial support for RCS keywords
From: Junio C Hamano @ 2012-02-23 20:03 UTC (permalink / raw)
To: Luke Diamand; +Cc: git, Pete Wyckoff, Eric Scouten
In-Reply-To: <1329983490-4033-2-git-send-email-luke@diamand.org>
Hmm... I somehow was expecting that the patch will be an incremental
update to what has already been queued in 'next' for the past several
days, with its commit log describing what the incremental improvements
are.
I'll revert it out and apply this on top. Thanks.
^ permalink raw reply
* Re: [PATCH] pretty: add '*' modifier to add LF after non-empty
From: Junio C Hamano @ 2012-02-23 19:53 UTC (permalink / raw)
To: Luc Pionchon; +Cc: git
In-Reply-To: <1330002637-9347-1-git-send-email-pionchon.luc@gmail.com>
Luc Pionchon <pionchon.luc@gmail.com> writes:
> Add the '*' modifier, similar to the '+' modifier,
> to add a line-feed after a non-empty placeholder.
Hrm, I thought I designed the plus and minus fairly carefully so that
nobody needs to add this later.
Wouldn't it be sufficient to write
Foo%n%-d
that says "We usually have LF after Foo, and write %d after that, but we
might not have anything interesting in %d at all, in which case we don't
add that %n"?
> +test_expect_success 'add LF after non-empty (1) (empty)' '
> + git show -s --pretty=format:"%*d%s%nfoo%n" HEAD^^ >actual &&
Shouldn't this be equivalent to "%n%-d%s%nfoo%n", which in turn is covered
by one of the previous tests (del LF before empty)?
^ permalink raw reply
* Re: git log -z doesn't separate commits with NULs
From: Jeff King @ 2012-02-23 19:34 UTC (permalink / raw)
To: Nikolaj Shurkaev; +Cc: Jakub Narebski, git
In-Reply-To: <4F4643BB.8090001@gmail.com>
On Thu, Feb 23, 2012 at 04:48:43PM +0300, Nikolaj Shurkaev wrote:
> For example there are commits that affect not only files in folder A
> but files in folder B, C and so on. If I do format-patch that will
> give me nice patches, but there are modifications of folders B, C and
> so on there. I do not know a way to generate patches via format-patch
> that affect only files in folder A.
Doesn't:
git format-patch HEAD~3..HEAD SomePath
do what you want? It is certainly designed to, and it seems to work for
me.
-Peff
^ permalink raw reply
* Re: [PATCH] merge: use editor by default in interactive sessions
From: Junio C Hamano @ 2012-02-23 19:23 UTC (permalink / raw)
To: kusmabite; +Cc: git
In-Reply-To: <CABPQNSZVOjOKpqv4s1ZCEQRd_yT3us3mqC9aN-KK5PHqztYQQg@mail.gmail.com>
Erik Faye-Lund <kusmabite@gmail.com> writes:
>> + /* Use editor if stdin and stdout are the same and is a tty */
>> + return (!fstat(0, &st_stdin) &&
>> + !fstat(1, &st_stdout) &&
>> + isatty(0) &&
>> + st_stdin.st_dev == st_stdout.st_dev &&
>> + st_stdin.st_ino == st_stdout.st_ino &&
>> + st_stdin.st_mode == st_stdout.st_mode);
>
> I just stumbled over this code, and I got a bit worried; the
> stat-implementation we use on Windows sets st_ino to 0, so
> "st_stdin.st_ino == st_stdout.st_ino" will always evaluate to true.
>
> Perhaps we should add a check for isatty(1) here as well? ...
> Or is there something I'm missing here?
No, the intention was not "we do this whether standard output is tty or
not", but was "we check that fd#0 and fd#1 are connected to the same
device by trusting stat() to do the right thing, so checking isatty(0)
is sufficient". As that "trusting stat()" assumption does not hold for
your platform, we would need to add isatty(1) to accomodate it.
Thanks for a set of sharp eyes.
^ permalink raw reply
* RE: gitk: set uicolor SystemButtonFace error on X11 if .gitk created using Win32 tk
From: Matt Seitz (matseitz) @ 2012-02-23 19:20 UTC (permalink / raw)
To: git; +Cc: kusmabite
In-Reply-To: <CABPQNSZqX0_YAn=mOpuRquG9OzFgwS9fQ6=YXqULxMz-hbH6Zw@mail.gmail.com>
> From: Erik Faye-Lund [mailto:kusmabite@gmail.com]
>
> Are you saying that the tk shipped with recent Cygwin reports "win32"
> for "[tk windowingsystem]", but does not recognize the
> "SystemButtonFace" etc values?
Here is what I am saying:
1. Cygwin tk 8.4.x and earlier:
-Reports "win32" for "[tk windowingsystem]"
-Recognizes the "SystemButtonFace" etc values
2. Cygwin tk 8.5.11:
-Reports "x11" for "[tk windowingsystem]"
-Does not recognize the "SystemButtonFace" etc values
> Does this patch help?
No, this has the same problem: if gitk is installed using a tk that
reports "win32" for "[tk windowingsystem]", then gitk hard codes win32
specific values into .gitk. If the user later changes to a tk that
doesn't support win32 specific values, gitk will no longer work.
Two use cases:
1. My situation: User replaces a win32 tk with an x11 tk
2. User uses a common .gitk file when running Windows and Linux.
Here's the error:
$ uname -a
CYGWIN_NT-5.1 matseitz-wxp 1.7.10(0.259/5/3) 2012-02-05 12:36 i686
Cygwin
$ wish
% tk windowingsystem
x11
% exit
$ gitk &
[1] 6312
$ Error in startup script: unknown color name "ButtonFace"
while executing
"winfo rgb . $c"
(procedure "setui" line 3)
invoked from within
"setui $uicolor"
(file "/usr/bin/gitk" line 11522)
^ permalink raw reply
* Re: [PATCH] remote-curl: Fix push status report when all branches fail
From: Junio C Hamano @ 2012-02-23 19:11 UTC (permalink / raw)
To: Jeff King; +Cc: Shawn Pearce, git
In-Reply-To: <20120223100434.GA3083@sigill.intra.peff.net>
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/4] Making an elephant out of a getline() bug
From: Junio C Hamano @ 2012-02-23 19:04 UTC (permalink / raw)
To: Thomas Rast; +Cc: Johannes Sixt, Jeff King, Jannis Pohlmann, git
In-Reply-To: <cover.1329988335.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> So you all made very good points, and I don't want to repeat them.
I really wish you did not make an elephant out of this, or if you must,
kept the minimum bugfix part that must apply to older codebase and making
an elephant part clearly separable.
Anyway, I've reordered so that the bugfix part would apply to an older
maintenance release, and then the remainder applied on top would merge
cleanly to the bleeding edge, so there is no need to resend.
Thanks for working on this, everybody.
^ permalink raw reply
* Re: [PATCH v2] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-23 19:02 UTC (permalink / raw)
To: Tim Henigan; +Cc: git, davvid, stefano.lattarini
In-Reply-To: <CAFouetiSpsZGtLt2tG4ou-H18zigNx5xWQH4cy8GrL1eDxbjJw@mail.gmail.com>
Tim Henigan <tim.henigan@gmail.com> writes:
> On Wed, Feb 22, 2012 at 6:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Another thing that comes to my mind is if a user has an external tool that
>> can use "diffall", is there ever a situation where the user chooses to use
>> "difftool" instead, to go files one by one. I cannot offhand imagine any.
>
> It was my assumption that any tool that supports directory diff also
> supports individual file diff. It seems like a strict subset of
> directory diff case.
> ...
>> Perhaps a longer term integration plan may be to lift the logic from this
>>
> ...snip...
>>
>> But that is all two steps in the future.
>
> I hope that this feature finds it way into the existing core commands.
> This script is intended to be a conversation starter that is also
> immediately useful as a separate command. Would it be better to begin
> the long-term discussion now and skip adding this to contrib?
I would envision we have this in contrib/ first, without even fixing the
whitespace-in-pathspec and whitespace-or-lf-in-paths issues I pointed out
in my review, and let people play with it.
My crystal ball tells an optimist in me that we will see people (you and
others) try to fix issues they hit in their real life use cases, and the
script will be improved while it is still in contrib/. And then somebody
who has worked on difftool will step up and roll it into difftool proper,
along the lines I hinted in the message you are responding to, at which
point the script will be removed from contrib/. That is the first step in
the future.
The second step in the future may or may not come. It will involve adding
an updated external diff interface on the core side and would prepare the
two temporary directories before the core side calls the difftool among
other things, and when that happens, we can lose most of the code in this
script that the first step in the future may have ported into difftool.
>> You can write multiple assignment on a line to save vertical space if you
>> want to, and the initialization sequence like this is a good place to do
>> so.
> My personal preference is to keep them on separate lines. However if
> the compressed style is preferred, I will change it.
"I wouldn't bother" was what I meant by "if you want to".
>>> + -x|--e|--ex|--ext|--extc|--extcm|--extcmd)
> ...
>> Don't you want to match "-t/--tool" that "difftool" already uses?
>
> Are you suggesting that I a) change "-x/--extcmd" to "-t/--tool" or
> that b) I add support for the "difftool -t/--tool" option?
No, I just misread the set of options "difftool" takes, without realizing
that --extcmd and --tool are two different things, and it is correct to
call your option "--extcmd" if it specifies what corresponds to what
"difftool --extcmd" specifies. Sorry for the confusion.
>> Hrm, so "diffall HEAD~2 Documentation/" is not the way to compare the
>> contents of the Documentation/ directory between the named commit and
>> the working tree, like "diff HEAD~2 Documentation/" does.
>>
>> That is not a show-stopper (a double-dash is an easy workaround), but
>> it is worth pointing out.
>
> So I would need something to determine if a string represents a
> commit/tag/branch or a path?.
"It does not have to be fixed in the first version" (aka "for future
consideration") was what I meant by "not a show-stopper".
>> And this will not work with pathspec that have $IFS characters. If we
>> really wanted to we could do that by properly quoting "$1" when you build
>> $paths and then use eval when you run "git diff" here (look for 'sq' and
>> 'eval' in existing scripts, e.g. "git-am.sh", if you are interested).
>>
>> Also you may want to write filelist using -z format to protect yourself
>> from paths that contain LF, but that would require the loop "while read
>> name" to be rewritten.
>
> I just discovered that the script fails to handle files that have
> spaces in their name, so some further work is needed.
Again, "It does not have to be fixed in the first version" (aka "for
future consideration") was what I meant by "If we really wanted to".
>> What is this "--ignore-failed-read" about? Not reporting unreadable as an
>> error smells really bad.
>
> If a file was added or deleted between the two commits being compared,
> tar would fail because a file was missing from "$tmp/filelist". The
> "--ignore-failed-read" prevents tar from halting the script in this
> case.
But it will also ignore errors coming from other causes, no? Wouldn't we
rather see an error if tar fails to read from a path that *has to* exist
in the working tree because "diff" said it does?
Again, it is just "for future consideration".
>> If you require GNUism in your tar usage, this should be made configurable
>> so that people can use alternative names (some systems come with "tar"
>> that is POSIX and "gtar" that is GNU).
>
> Is there an example showing how this could be configurable? The
> problem is that the "--ignore-failed-read" was not supported in all
> flavors of tar.
Grep for "$TAR" and also @@DIFF@@ in the Makefile, and add substitution
for @@TAR@@ in cmd_munge_script, perhaps?
By the way, I actually have an even more radical suggestion that may let
you get rid of most of the lines in your script.
If you tweak the usage so that "diffall" specific options *MUST* come
first, e.g.
USAGE=[--copy-back] [-x <cmd>] <arguments for diff>
then you can parse your argument partially, i.e.
copy_back= extcmd=
while case "$#,$1" in 0,*) break ;; *,-*) ;; *) break ;; esac
do
case "$1" in
--copy-back)
copy_back=true
;;
-x | --extcmd)
test $# != 1 || usage
extcmd=$2
shift
;;
*)
break
;;
esac
shift
done
then feed the remainder all to "diff", e.g.
diff --raw --no-abbrev "$@" |
And then you can prepare two temporary index files and stuff the output in
them, by having something like this on the downstream of the pipe:
while read -r lmode rmode lsha1 rsha1 status path
do
if test "$lmode" != $null_mode
then
GIT_INDEX_FILE=$tmp.left_index \
git update-index --add --cacheinfo $lmode $lsha1 $path
fi
if test "$rmode" != $null_mode
then
GIT_INDEX_FILE=$tmp.right_index \
git update-index --add --cacheinfo $rmode $rsha1 $path
fi
done
Side Note:
In the production version, you would probably give the "-z" option
to "diff", and write this loop in Perl so that you can cope better
with funny characters in the path. Instead of running two
instances of "git update-index" for every path, the loop would
group the entries for left and right side, and drive one instance
of "git update-index --index-info" each to populate the two index
files.
Also the above needs to be adjusted to deal with the side that
represents the working tree files; they are reported with $null_sha1
so in such a case instead of putting it in the temporary index,
you would copy the working tree file to the temporary location.
After you prepare these two temporary index files, you can then use them
to populate your left_dir and right_dir like this:
GIT_DIR=$(git rev-parse --git-dir) \
GIT_WORK_TREE=$left_dir \
GIT_INDEX_FILE=$tmp.left_index \
git checkout-index -a
With this, you do not have to worry about anything about the funny
combinations of where the two "directories" comes from when preparing
the temporary directories to be compared.
^ permalink raw reply
* Integration with UDDI-based registries and repositories
From: Lico Galindo @ 2012-02-23 18:48 UTC (permalink / raw)
To: git
Good afternoon,
We use the Reusable Component Services (RCS), a registry/repository
solution based on Software AG's CentraSite (a UDDI-based registry) for
registration of programming code and other IT assets. Code is registered
in RCS and may be stored either in the RCS repository or anywhere the
owner wants (storing the link in the registry). Some of the developers
have asked us to explore integrating RCS with Git or GitHub. This wiould
include loading code registered in RCS into Git, modifying the code in
whatever tool and registering the modified code back into RCS. By the
way, RCS supports developer interfaces like Eclipse and Visual Studio.
Does this make sense to you or am I misunderstanding your tool? How do
you think this would work?
Thanks,
Lico Galindo, PMP
IT Specialist
Data Standards Branch
Office of Environmental Information
Phone: 202.566.1252
Fax: 202.566.1684
^ permalink raw reply
* Re: [PATCH v2] contrib: added git-diffall
From: Junio C Hamano @ 2012-02-23 17:37 UTC (permalink / raw)
To: Stefano Lattarini; +Cc: Tim Henigan, git, davvid
In-Reply-To: <4F460D45.7000804@gmail.com>
Stefano Lattarini <stefano.lattarini@gmail.com> writes:
>>> +# mktemp is not available on all platforms (missing from msysgit)
>>> +# Use a hard-coded tmp dir if it is not available
>>> +tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
>>> + tmp=/tmp/git-diffall-tmp
>>> +}
>> ...
> # mktemp is not available on all platforms (missing from msysgit)
> tmp=$(mktemp -d -t tmp.XXXXXX 2>/dev/null) || {
> tmp=/tmp/git-diffall-tmp.$$
> mkdir "$tmp" || fatal "couldn't create temporary directory"
> }
>
>>> +mkdir -p "$tmp"
>>
> At which point this should be removed, of course.
Good eyes; thanks.
^ permalink raw reply
* Submodule commits not show by git log
From: Wilfred Hughes @ 2012-02-23 16:49 UTC (permalink / raw)
To: git
Hi all,
I'm trying to track changes on a git submodule. `$ git log
path/to/submodule` is not showing any commits that changed the
submodule commit.
For example, in my repo I have a submodule at the path
./memcache_utils that has had the commit referenced changed several
times. It's definitely a submodule:
$ git submodule
13eb087304b995705693d6f0927dec2d88dfadaf datastore_utils
(heads/master-2-g13eb087)
7f5d6710b767a27f14e3e7bc009f026b3e5f0e74 memcache_utils (heads/master)
5877e2c2d82645fa44f121884291ee48cf24584d potatobase (5877e2c)
Yet the only commit shown is when there were files at that path:
$ git log memcache_utils
commit 2cbe65bf31901873b01331e95fec72724e7458eb
Author: [...]
Date: Thu Jan 26 20:44:07 2012 +0000
Experimenting with Paul G's port of cache-machine. It should
really be a git submodule but just want to see how it works so have
taken a copy from [...]
$
Have I missed something? I've tried and failed to create a minimal
test case that demonstrates this behaviour. I can't see anything in
the man pages to suggest that I'm using git log incorrectly, and
Googling doesn't help.
Is this user error, a known bug, or a new one?
Many thanks
Wilfred
^ permalink raw reply
* Re: [PATCH v2] contrib: added git-diffall
From: Tim Henigan @ 2012-02-23 16:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, davvid, stefano.lattarini
In-Reply-To: <7vipiy8m5q.fsf@alter.siamese.dyndns.org>
On Wed, Feb 22, 2012 at 6:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
> We encourage our log messages to describe the problem first and then
> present solution to the problem, so I would update the above perhaps like
> this:
>
> The 'git difftool' command lets the user to use an external tool
> to view diffs, but it runs the tool for one file at the time. This
> makes it tedious to review a change that spans multiple files.
>
> The "git-diffall" script instead prepares temporary directories
> with preimage and postimage files, and launches a single instance
> of an external diff tool to view the differences in them.
> diff.tool or merge.tool configuration variable is used to specify
> what external tool is used.
Understood. I will update in v3.
> I am wondering if reusing "diff.tool" or "merge.tool" is a good idea,
> though.
>
> I guess that it is OK to assume that any external tool that can compare
> two directories MUST be able to compare two individual files, and if that
> is true, it is perfectly fine to reuse the configuration. But if an
> external tool "frobdiff" that can compare two directories cannot compare
> two individual files, it will make it impossible for the user to run "git
> difftool" if diff.tool is set to "frobdiff" to use with "diffall".
>
> Another thing that comes to my mind is if a user has an external tool that
> can use "diffall", is there ever a situation where the user chooses to use
> "difftool" instead, to go files one by one. I cannot offhand imagine any.
It was my assumption that any tool that supports directory diff also
supports individual file diff. It seems like a strict subset of
directory diff case.
> Perhaps a longer term integration plan may be to lift the logic from this
>
...snip...
>
> But that is all two steps in the future.
I hope that this feature finds it way into the existing core commands.
This script is intended to be a conversation starter that is also
immediately useful as a separate command. Would it be better to begin
the long-term discussion now and skip adding this to contrib?
>> +# mktemp is not available on all platforms (missing from msysgit)
>> +# Use a hard-coded tmp dir if it is not available
>> +tmp="$(mktemp -d -t tmp.XXXXXX 2>/dev/null)" || {
>> + tmp=/tmp/git-diffall-tmp
>> +}
>
> It would not withstand malicious attacks, but doing
>
> tmp=/tmp/git-diffall-tmp.$$
>
> would at least protect you from accidental name crashes better in the
> fallback codepath.
This makes sense. I will add a unique portion to the name of the tmp dir in v3.
>> +trap 'rm -rf "$tmp" 2>/dev/null' EXIT
>
> Do you need to suppress errors, especially when you are running "rm -rf"
> with the 'f' option?
On msysgit, I found that "rm -rf $tmp" consistently fails due with a
permission error. I don't understand why the script that created the
tmp dir is not allowed to delete it. I am still looking into it, but
so far it appears to be an idiosyncrasy of msysgit.
>> +left=
>> +right=
>> +paths=
>> +path_sep=
>> +compare_staged=
>> +common_ancestor=
>> +left_dir=
>> +right_dir=
>> +diff_tool=
>> +copy_back=
>
> You can write multiple assignment on a line to save vertical space if you
> want to, and the initialization sequence like this is a good place to do
> so.
My personal preference is to keep them on separate lines. However if
the compressed style is preferred, I will change it.
>> + -x|--e|--ex|--ext|--extc|--extcm|--extcmd)
>> + diff_tool=$2
>> + shift
>> + ;;
>
> What if your command line ends with -x without $2?
Currently it results in a shift error with no useful message to the
user. I will add something for this in v3.
> Don't you want to match "-t/--tool" that "difftool" already uses?
Are you suggesting that I a) change "-x/--extcmd" to "-t/--tool" or
that b) I add support for the "difftool -t/--tool" option?
If "a", I was reusing the "difftool --extcmd" option which has the
same behavior. If "b", I will look into it.
>> + # could be commit, commit range or path limiter
>> + case "$1" in
>> + *...*)
>> + left=${1%...*}
>> + right=${1#*...}
>> + common_ancestor=1
>> + ;;
>
> Strictly speaking, that is not just a common_ancestor but is a merge_base,
> which is a common ancestor none of whose children is a common ancestor.
Understood. I will change the name to "merge_base" in v3.
>> + *..*)
>> + left=${1%..*}
>> + right=${1#*..}
>> + ;;
>> + *)
>> + if test -n "$path_sep"
>> + then
>> + paths="$paths$1 "
>> + elif test -z "$left"
>> + then
>> + left=$1
>> + elif test -z "$right"
>> + then
>> + right=$1
>> + else
>> + paths="$paths$1 "
>> + fi
>> + ;;
>> + esac
>
> Hrm, so "diffall HEAD~2 Documentation/" is not the way to compare the
> contents of the Documentation/ directory between the named commit and
> the working tree, like "diff HEAD~2 Documentation/" does.
>
> That is not a show-stopper (a double-dash is an easy workaround), but
> it is worth pointing out.
So I would need something to determine if a string represents a
commit/tag/branch or a path?. I presume it would need to handle the
corner case where a branch/tag and path have the same name. Is there
anything like this in the mergetool lib scripts today?
>> + then
>> + git diff --name-only "$left"..."$right" -- $paths > "$tmp/filelist"
>> + else
>> + git diff --name-only "$left" "$right" -- $paths > "$tmp/filelist"
>> + fi
>
> And this will not work with pathspec that have $IFS characters. If we
> really wanted to we could do that by properly quoting "$1" when you build
> $paths and then use eval when you run "git diff" here (look for 'sq' and
> 'eval' in existing scripts, e.g. "git-am.sh", if you are interested).
>
> Also you may want to write filelist using -z format to protect yourself
> from paths that contain LF, but that would require the loop "while read
> name" to be rewritten.
I just discovered that the script fails to handle files that have
spaces in their name, so some further work is needed.
>> +# Exit immediately if there are no diffs
>> +if test ! -s "$tmp/filelist"
>> +then
>> + exit 0
>> +fi
>
> Ok, you have trap set already so $tmp will disappear with this exit ;-)
I'll try not to be such a slow learner in the future...but no guarantees.
>> +if test -n "$copy_back" && test "$right_dir" != "working_tree"
>> +then
>> + echo "--copy-back is only valid when diff includes the working tree."
>> + exit 1
>> +fi
>
> I actually wondered why $right_dir needs to be populated with a copy in
> the first place (if you do not copy but give the working tree itself to
> the external tool, you do not even have to copy back).
>
> I know the answer to the question, namely, "because the external tool
> thinks files that are not in $left_dir are added files", but if you can
> find a way to tell the external tool to ignore new files (similar to how
> "diff -r" without -N works), running the tool with temporary left_dir and
> the true workdir as right_dir would be a lot cleaner solution to the
> problem.
I'll note this as "for future consideration". I spent some time
trying this is the original implementation, but could not find a
workable solution in the time I had available.
>> + while read name
>> + do
>> + ls_list=$(git ls-tree $right $name)
>> + if test -n "$ls_list"
>> + then
>> + mkdir -p "$tmp/$right_dir/$(dirname "$name")"
>> + git show "$right":"$name" > "$tmp/$right_dir/$name" || true
>> + fi
>> + done < "$tmp/filelist"
>
> "while read -r name" might make this slightly more robust; even though
> this loses leading and trailing whitespaces in filenames, we probably
> can get away without worrying about them.
>
>> +else
>> + # Mac users have gnutar rather than tar
>> + (tar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && tar -x)) || {
>> + gnutar --ignore-failed-read -c -T "$tmp/filelist" | (cd "$tmp/$right_dir" && gnutar -x)
>> + }
>
> What is this "--ignore-failed-read" about? Not reporting unreadable as an
> error smells really bad.
If a file was added or deleted between the two commits being compared,
tar would fail because a file was missing from "$tmp/filelist". The
"--ignore-failed-read" prevents tar from halting the script in this
case.
> If you require GNUism in your tar usage, this should be made configurable
> so that people can use alternative names (some systems come with "tar"
> that is POSIX and "gtar" that is GNU).
Is there an example showing how this could be configurable? The
problem is that the "--ignore-failed-read" was not supported in all
flavors of tar.
>> +# Copy files back to the working dir, if requested
>> +if test -n "$copy_back" && test "$right_dir" = "working_tree"
>> +then
>> + cd "$start_dir"
>> + git_top_dir=$(git rev-parse --show-toplevel)
>> + find "$tmp/$right_dir" -type f |
>> + while read file
>> + do
>> + cp "$file" "$git_top_dir/${file#$tmp/$right_dir/}"
>> + done
>> +fi
>
> This will copy new files created in $right_dir. Is that intended?
hmmm...that was not intended. If would be odd for the user to create
new files in this tmp directory, but if the diff tool automatically
generates any files then this could result in unwanted files.
^ permalink raw reply
* [RFC/PATCH] Make git-{pull,rebase} no-tracking message friendlier
From: Carlos Martín Nieto @ 2012-02-23 16:05 UTC (permalink / raw)
To: git
The current message is too long and at too low a level for anybody to
understand it if they don't know about the configuration format
already.
Reformat it to show the commands a user would be expected to use,
instead of the contents of the configuration file.
---
As annoying as it is when people simply paste the output of 'git pull'
and ask "what does it mean" without even reading it, I have to admit
that as a new user, I'd also be scared off by this message. Using
git-remote and git-branch should make it less scary and more relatable
for the user. I'm not aware of a way to set branch.$branch.rebase with
porcelain, so I put in a config command there. Better
solutions/wordings welcome; I'd really like to get rid of the old
message.
git-parse-remote.sh | 23 ++++++++---------------
1 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index b24119d..96e76a9 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -65,26 +65,19 @@ Please specify which branch you want to $op_type $op_prep on the command
line and try again (e.g. '$example').
See git-${cmd}(1) for details."
else
- echo "You asked me to $cmd without telling me which branch you
-want to $op_type $op_prep, and 'branch.${branch_name#refs/heads/}.merge' in
-your configuration file does not tell me, either. Please
-specify which branch you want to use on the command line and
+ echo "You asked me to $cmd without telling me which branch you want to
+$op_type $op_prep, and there is no tracking information for the current branch.
+Please specify which branch you want to use on the command line and
try again (e.g. '$example').
See git-${cmd}(1) for details.
If you often $op_type $op_prep the same branch, you may want to
-use something like the following in your configuration file:
- [branch \"${branch_name#refs/heads/}\"]
- remote = <nickname>
- merge = <remote-ref>"
- test rebase = "$op_type" &&
- echo " rebase = true"
- echo "
- [remote \"<nickname>\"]
- url = <url>
- fetch = <refspec>
+run something like:
-See git-config(1) for details."
+ git remote add <remote> <url>
+ git branch --set-upstream ${branch_name#refs/heads/} <remote>/<remote-branch>"
+ test rebase = "$op_type" &&
+ echo " git config branch.${branch_name#refs/heads/}.rebase true"
fi
exit 1
}
--
1.7.8.352.g876a6f
^ permalink raw reply related
* [PATCHv3 3/3] gitweb: Faster project search
From: Jakub Narebski @ 2012-02-23 15:54 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jakub Narebski
In-Reply-To: <1330012488-7970-1-git-send-email-jnareb@gmail.com>
Before searching by some field the information we search for must be
filled in, but we do not have to fill other fields that are not
involved in the search.
To be able to request filling only specified fields,
fill_project_list_info() was enhanced in previous commit to take
additional parameters which specify part of projects info to fill.
This way we can limit doing expensive calculations (like running
git-for-each-ref to get 'age' / "Last changed" info) to doing those
only for projects which we will show as search results.
This commit actually uses this interface, changing gitweb code from
the following behavior
fill all project info on all projects
search projects
to behaving like this pseudocode
fill search fields on all projects
search projects
fill all project info on search results
With this commit the number of git commands used to generate search
results is 2*<matched projects> + 1, and depends on number of matched
projects rather than number of all projects (all repositories).
Note: this is 'git for-each-ref' to find last activity, and 'git config'
for each project, and 'git --version' once.
Example performance improvements, for search that selects 2
repositories out of 12 in total:
* Before (warm cache):
"This page took 0.867151 seconds and 27 git commands to generate."
* After (warm cache):
"This page took 0.673643 seconds and 5 git commands to generate."
Now imagine that they are 5 repositories out of 5000, and cold or
trashed cache case.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Changes from v2 (v2 is the same as v1):
* Rewritten and extended commit message (though extending perhaps have
gone too far...).
Added paragraphs are "This commit actually uses this interface..."
and "Example performance improvements..."
Junio C Hamano wrote in
http://thread.gmane.org/gmane.comp.version-control.git/190852/focus=191053
[...]
JC> "must be filled in." is correct, but that happens without the previous
JC> patch. The first sentence must also say:
JC>
JC> In order to search by some field, the information we look for must
JC> be filled in, but we do not have to fill other fields that are not
JC> involved in the search.
JC>
JC> to justify the previous "fill_project_list_info can be asked to return
JC> without getting unused fields" patch.
Done. Thanks for a suggestion.
JC> The rest of the log message then makes good sense.
Ooops...
gitweb/gitweb.perl | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7fb7a55..4ceb1a6 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -2987,6 +2987,10 @@ sub search_projects_list {
return @$projlist
unless ($tagfilter || $searchtext);
+ # searching projects require filling to be run before it;
+ fill_project_list_info($projlist,
+ $tagfilter ? 'ctags' : (),
+ $searchtext ? ('path', 'descr') : ());
my @projects;
PROJECT:
foreach my $pr (@$projlist) {
@@ -5394,12 +5398,13 @@ sub git_project_list_body {
# filtering out forks before filling info allows to do less work
@projects = filter_forks_from_projects_list(\@projects)
if ($check_forks);
- @projects = fill_project_list_info(\@projects);
- # searching projects require filling to be run before it
+ # search_projects_list pre-fills required info
@projects = search_projects_list(\@projects,
'searchtext' => $searchtext,
'tagfilter' => $tagfilter)
if ($tagfilter || $searchtext);
+ # fill the rest
+ @projects = fill_project_list_info(\@projects);
$order ||= $default_projects_order;
$from = 0 unless defined $from;
--
1.7.9
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox