Git development
 help / color / mirror / Atom feed
* Wishlist: git help bisect should mention "stop" keyword
From: Andreas Mohr @ 2013-02-09  8:44 UTC (permalink / raw)
  To: git

Hi,

the man page (git version 1.7.10.4) is a bit non-symmetric
since git bisect has the start param, but when searching for "stop"
(nothing more obvious than that, right?),
one comes up empty --> usability issue.

The appropriate action complementary to start appears to be
git bisect reset, thus its description definitely ought to include a "stop"
keyword.

Description as of 1.7.10.4 is

       After a bisect session, to clean up the bisection state and
return to
       the original HEAD, issue the following command:

           $ git bisect reset

which could be changed into

       After a bisect session, to clean up the bisection state and
return to
       the original HEAD (in other words, to "stop" bisect),
       issue the following command:

           $ git bisect reset

Andreas Mohr

-- 
GNU/Linux. It's not the software that's free, it's you.

^ permalink raw reply

* Re: Credentials and the Secrets API...
From: John Szakmeister @ 2013-02-09 10:58 UTC (permalink / raw)
  To: Ted Zlatanov; +Cc: git
In-Reply-To: <87halochci.fsf@lifelogs.com>

On Thu, Feb 7, 2013 at 9:46 AM, Ted Zlatanov <tzz@lifelogs.com> wrote:
> On Thu, 27 Oct 2011 12:05:03 -0400 John Szakmeister <john@szakmeister.net> wrote:
>
> JS> Just wanted to keep folks in the loop.  It turns out that the Secrets
> JS> API is still to young.  I asked about the format to store credentials
> JS> in (as far as attributes), and got a response from a KDE developer
> JS> that says it's still to young on their front.  They hope to have
> JS> support in the next release of KDE.  But there's still the issue of
> JS> what attributes to use.
>
> JS> With that information, I went ahead and created a
> JS> gnome-credential-keyring that uses Gnome's Keyring facility.  I still
> JS> need to do a few more things (mainly run it against Jeff's tests), but
> JS> it's generally working.  Just wanted to keep folks in the loop.
> JS> Hopefully, I can get patches out this weekend.
>
> Do you think the Secrets API has matured enough?  KDE has had a new
> release since your post...

Yes, I think it has.  Several other applications appear to be using
it, including some things considered "core" in Fedora--which is a good
sign.

-John

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 11:32 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Duy Nguyen
In-Reply-To: <7vsj56w5y9.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I checked read-cache.c and preload-index.c code.  To get the
>> discussion rolling, I think something like the outline below may be
>> a good starting point and a feasible weekend hack for somebody
>> competent:
>>
>>  * At the beginning of preload_index(), instead of spawning the
>>    worker thread and doing the lstat() check ourselves, we open a
>>    socket to our daemon (see below) that watches this repository and
>>    make a request for lstat update.  The request will contain:
>>
>>     - The SHA1 checksum of the index file we just read (to ensure
>>       that we and our daemon share the same baseline to
>>       communicate); and
>>
>>     - the pathspec data.
>>
>>    Our daemon, if it already has a fresh data available, will give
>>    us a list of <path, lstat result>.  Our main process runs a loop
>>    that is equivalent to what preload_thread() runs but uses the
>>    lstat() data we obtained from the daemon.  If our daemon says it
>>    does not have a fresh data (or somehow our daemon is dead), we do
>>    the work ourselves.
>>
>>  * Our daemon watches the index file and the working tree, and
>>    waits for the above consumer.  First it reads the index (and
>>    remembers what it read), and whenever an inotify event comes,
>>    does the lstat() and remembers the result.  It never writes
>>    to the index, and does not hold the index lock.  Whenever the
>>    index file changes, it needs to reload the index, and discard
>>    lstat() data it already has for paths that are lost from the
>>    updated index.
>
> I left the details unsaid in thee above because I thought it was
> fairly obvious from the nature of the "outline", but let me spend a
> few more lines to avoid confusion.
>
>  - The way the daemon "watches" the changes to the working tree and
>    the index may well be very platform dependent.  I said "inotify"
>    above, but the mechanism does not have to be inotify.

Is the BSD kernel's inotify the same as the one on Linux?  Must we
design something that's generic enough from the start?

More importantly, do you know of a platform-independent inotify
implementation in C?  A quick Googling turned up QFileSystemWatcher
[1], a part of QT.

[1]: http://qt-project.org/doc/qt-4.8/qfilesystemwatcher.html

>  - The channel the daemon and the client communicates would also be
>    system dependent.  UNIX domain socket in $GIT_DIR/ with a
>    well-known name would be one possibility but it does not have to
>    be the only option.

UNIX domain sockets are also preferred because we'd never want to
connect to a watch daemon over the network?

Then the communication channel code also has to be generic enough.

>  - The data given from the daemon to the client does not have to
>    include full lstat() information.  They start from the same index
>    info, and the only thing preload_index() wants to know is for
>    which paths it should call ce_mark_uptodate(ce), so the answer
>    given by our daemon can be a list of paths.

Right.

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 12:05 UTC (permalink / raw)
  To: Robert Zeh; +Cc: Junio C Hamano, Git List, Duy Nguyen
In-Reply-To: <9AF8A28B-71FE-4BBC-AD55-1DD3FDE8FFC3@gmail.com>

Robert Zeh wrote:
> From the description so far, I have some question: how does the daemon get started and stopped?  Is there one per repository ...

What about getting systemd to watch everything for us?  Then we can
just have one daemon reporting filesystem changes over one global
socket.  It's API should be the inotify subset:

   systemd_add_watch
   systemd_remove_watch

Except systemd_add_watch also accepts a UNIX socket to send lstat
events to.  Our preload_index() is just reduced to making one
systemd_add_watch() call the very first time and updating the index as
necessary.  Now, what about desktops with huge uptimes (like mine)?
Won't they get polluted with too many useless watches over time?
Simple: timeout.  If nobody reads from the UNIX socket for two hours
after a systemd_add_watch, execute systemd_remove_watch automatically.

Someone must implement a similar daemon on other platforms reporting
information in exactly the same way (although with different
internals).  IP sockets are system-wide and all platforms have them,
so the communication channel is also standardized.

This is much better than Junio's suggestion to study possible
implementations on all platforms and designing a generic daemon/
communication channel.  That's no weekend project.

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 12:11 UTC (permalink / raw)
  To: Robert Zeh; +Cc: Junio C Hamano, Git List, Duy Nguyen
In-Reply-To: <CALkWK0mttn6E+D-22UBbvDCuNEy_jNOtBaKPS-a8mTbO2uAF3g@mail.gmail.com>

Ramkumar Ramachandra wrote:
> What about getting systemd to watch everything for us?  Then we can
> just have one daemon reporting filesystem changes over one global
> socket.  It's API should be the inotify subset:

Er, not one global socket: many little sockets as described later.
(The idea was just forming while I was writing this paragraph)

^ permalink raw reply

* [BUG] can't switch branches with submodules
From: Ramkumar Ramachandra @ 2013-02-09 12:25 UTC (permalink / raw)
  To: Git List; +Cc: Jens Lehmann

Hi,

I have two branches: master and gh-pages.  master has one submodule
called foo that gh-pages doesn't.  When I try to check out gh-pages
from master:

    warning: unable to rmdir foo: Directory not empty

And the foo directory exists in my worktree.  This is very annoying,
and I want to fix it now.  Where is this error coming from?  How does
the worktree get updated when I checkout?

^ permalink raw reply

* [BUG] can't switch branches with submodules
From: W. Trevor King @ 2013-02-09 12:32 UTC (permalink / raw)
  To: Ramkumar Ramachandra
  Cc: Jens Lehmann, Jonathan Nieder, Peter Collingbourne, Git
In-Reply-To: <50EA7269.1080006@web.de>

[-- Attachment #1: Type: text/plain, Size: 1071 bytes --]

On Sat, Feb 09, 2013 at 05:55:26PM +0530, Ramkumar Ramachandra wrote:
> I have two branches: master and gh-pages.  master has one submodule
> called foo that gh-pages doesn't.  When I try to check out gh-pages
> from master:
> 
>     warning: unable to rmdir foo: Directory not empty
> 
> And the foo directory exists in my worktree.  This is very annoying,
> and I want to fix it now.  Where is this error coming from?  How does
> the worktree get updated when I checkout?

You may want to look at the pending Great Submodule Integration
Branch:

On Mon, Jan 07, 2013 at 07:59:53AM +0100, Jens Lehmann wrote:
> I´m currently working on teaching mv to move submodules and intend
> to send those patches to the list after finishing submodule deinit.
> Please see
>   https://github.com/jlehmann/git-submod-enhancements/commits/mv-submodules
> for the current state of this series.

;)
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 12:53 UTC (permalink / raw)
  To: Robert Zeh; +Cc: Junio C Hamano, Git List, Duy Nguyen
In-Reply-To: <CALkWK0mttn6E+D-22UBbvDCuNEy_jNOtBaKPS-a8mTbO2uAF3g@mail.gmail.com>

Ramkumar Ramachandra wrote:
> What about getting systemd to watch everything for us?

systemd is the perfect candidate!  It already has an inotify watcher:
see systemd.path(5).  Can't be used as-is because it spawns processes
on events, which is a non-scalable design.  Secondly, it uses static
.path files to define the rules which is no good for us.  So, we need
to add an API to it, and ask it to report events over IP sockets.  The
API part is simple too, because it already has a DBUS API for many
things [1]; it's just a matter of extending it.

Yes, I know.  This introduces dbus as an additional optional
non-portable dependency.  Do you have suggestions for alternatives
that aren't complicated?

[1]: http://www.freedesktop.org/wiki/Software/systemd/dbus

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Duy Nguyen @ 2013-02-09 12:59 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Robert Zeh, Junio C Hamano, Git List
In-Reply-To: <CALkWK0nQVjKpyef8MDYMs0D9HJGCL8egypT3YWSdU8EYTO7Y+w@mail.gmail.com>

On Sat, Feb 9, 2013 at 7:53 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> What about getting systemd to watch everything for us?
>
> systemd is the perfect candidate!

How about this as a start? I did not really check what it does, but it
does not look complicate enough to pull systemd in.

http://article.gmane.org/gmane.comp.version-control.git/151934

Youo may want to search the mail archive. This topic has come up a few
times before, there may be other similar patches.
-- 
Duy

^ permalink raw reply

* Re: Feature request: Allow extracting revisions into directories
From: Robert Clausecker @ 2013-02-09 15:58 UTC (permalink / raw)
  To: git; +Cc: Phil Hord
In-Reply-To: <CABURp0rMk-W8VMRhXoR9YYQSwjWTfPbXz5mhPX3-HKsBSu5_mw@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 3443 bytes --]

After thinking a while about how to solve the problems I have, I
consider the following things as a solution to my problem.

Add an option --isolated, -i to git checkout: Check out a branch / tag /
revision but do not touch the index. This could be used together with
--work-tree to check out a branch into an arbitrary directory. Also, it
satisfies all 4 criteria from [1] and therefore is perfect for
deployment from a bare repository.

What do you think about this feature request?

Yours, Robert Clausecker

[1]: http://sitaramc.github.com/the-list-and-irc/deploy.html

Am Dienstag, den 05.02.2013, 10:11 -0500 schrieb Phil Hord:
> On Sun, Feb 3, 2013 at 7:42 PM, Sitaram Chamarty <sitaramc@gmail.com> wrote:
> > On 02/03/2013 11:41 PM, Robert Clausecker wrote:
> >>
> >> Am Sonntag, den 03.02.2013, 21:55 +0530 schrieb Sitaram Chamarty:
> >>> Could you help me understand why piping it to tar (actually 'tar -C
> >>> /dest/dir -x') is not sufficient to achieve what you want?
> >>
> >> Piping the output of git archive into tar is of course a possible
> >> solution; I just don't like the fact that you need to pipe the output
> >> into a separate program to do something that should be possible with a
> >> simple switch and not an extra command. It feels unintuitive and like a
> >> workaround to make an archive just to unpack it on-the-fly. Also, adding
> >> such a command (or at least documenting the way to do this using a pipe
> >> to tar somewhere in the man pages) is a small and simple change that
> >> improves usability.
> >
> > I realise it appears to be the fashion these days to get away from the
> > Unix philosophy of having different tools do different things and
> > combining them as needed.
> >
> > Ignoring the option-heavy GNU, and looking at the more traditional BSD
> > tar manpage [1], I notice the following flags that could still be
> > potentially needed by someone running 'git archive': '-t' (instead of
> > '-x'), '-C dir', '--exclude/include', '-k', '-m', '--numeric-owner', -o,
> > -P, -p, -q, -s, -T, -U, -v, -w, and -X.
> 
> OP did not ask about tar so I do not see why any of these tar options
> are relevant.  It seems like what he really wants is 'git archive
> --format=native' , maybe.   You can almost create this option by
> saying
> 
>    git config tar.native.command "tar -x"
> 
> except that you do not get the opportunity to specify a target directory.
> 
> But maybe he really wants a form of 'git checkout' instead.
> 
> 
> > And I'm ignoring the esoteric ones like "--chroot" and "-S" (sparse mode).
> >
> > How many of these options would you like included in git?  And if you
> > say "I don't need any of those; I just need '-x'", that's not relevant.
> >  Someone else may need any or all of those flags, and if you accept "-x"
> > you have to accept some of the others too.
> 
> This is only true if you cannot stop yourself from thinking about
> 'tar'.  What about zip, for example?
> 
> I think none of these options is relevant.
> 
> 
> > Also, I often want to deploy to a different host, and I might do that
> > like so:
> >
> >     git archive ... | ssh host tar -C /deploy/dir -x
> >
> > Why not put that ssh functionality into git also?
> 
> This slippery-slope argument is growing tiresome.
> 
> Phil
> 
> p.s. Conceded: OP set off this avalanche by disparaging the vaunted
> PIPE operation.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 17:10 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Robert Zeh, Junio C Hamano, Git List, finnag
In-Reply-To: <CACsJy8CEHzqH1X=v4yau0SyZwrZp1r6hNp=yXD+eZh1q_BS-0g@mail.gmail.com>

Duy Nguyen wrote:
> How about this as a start? I did not really check what it does, but it
> does not look complicate enough to pull systemd in.
>
> http://article.gmane.org/gmane.comp.version-control.git/151934

Clever hack.  I didn't know that there was a switch called
core.ignoreStat which will disable automatic lstat() calls altogether.
 So, Finn advises that we set this switch and run igit instead of git.
 There's a git-inotify-daemon which runs inotifywait with -m forever,
updating a modified_files hash.  When it is sent a TERM from igit
(which is what happens immediately upon execution), it writes all this
collected information about modified files to a named pipe that igit
passes to it.  igit then does a git update-index --assume-unchained
--stdin to read the data from the pipe.  Towards the end of its life,
igit starts up a fresh git-inotify-daemon for future invocations.

Finn notes in the commit message that it offers no speedup, because
.gitignore files in every directory still have to be read.  I think
this is silly: we really should be caching .gitignore, and touching it
only when lstat() reports that the file has changed.

As far as a real implementation that we'd want to merge into git.git
is concerned, I have a few comments:
Running multiple daemons on-the-fly for monitoring filesystem changes
is not elegant at all.  Keeping track of the state of so many loose
daemons is a hard problem: how do we ensure any semblance of
reliability without that?  Systemd is a very big improvement over the
legacy of a hundred loose shell scripts that SysVInit demanded.  It
monitors and babysits daemons; it uses cgroups to even kill
misbehaving daemons.  I can inspect running daemons at any time, and
have a uniform way to start/ stop/ restart them.

Okay, now you're asking me to consider a system-wide daemon
independent of systemd.  It has to run with root privileges so it has
access to everyone's repositories, which means that people have to
trust it beyond doubt.  What does it do?  It has a generic API to
watch filesystem paths and report events over an IP socket.  Do you
think that this will only be useful to git?  Every other version
control system (and presumably many other pieces of software) will
want to use it.  One huge downside I see of making this part of
systemd is Ubuntu.  They've decided not to use systemd for some
unfathomable reason.

Really, the elephant in the room right now seems to be .gitignore.
Until that is fixed, there is really no use of writing this inotify
daemon, no?  Can someone enlighten me on how exactly .gitignore files
are processed?

> Youo may want to search the mail archive. This topic has come up a few
> times before, there may be other similar patches.

The thread you linked me to is a 2010 email, and now it's 2013.  We've
been silent about inotify for three years?

Thanks for your inputs, Duy.

^ permalink raw reply

* git-completion.bash --local
From: Dasa Paddock @ 2013-02-09 18:37 UTC (permalink / raw)
  To: git@vger.kernel.org

I think this line should include --local:

https://github.com/git/git/blob/next/contrib/completion/git-completion.bash#L1782
    "--global|--system|--file=*)"

This would help for:
    git config -l --local

Thanks,
Dasa Paddock

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Ramkumar Ramachandra @ 2013-02-09 18:56 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Robert Zeh, Junio C Hamano, Git List, Finn Arne Gangstad
In-Reply-To: <CALkWK0=6_n4rf6AWci6J+uhGHpjTUmK7YFdVHuSJedN2zLWtMA@mail.gmail.com>

Ramkumar Ramachandra wrote:
> Okay, now you're asking me to consider a system-wide daemon
> independent of systemd.  It has to run with root privileges so it has
> access to everyone's repositories, which means that people have to
> trust it beyond doubt.  What does it do?  It has a generic API to
> watch filesystem paths and report events over an IP socket.  Do you
> think that this will only be useful to git?  Every other version
> control system (and presumably many other pieces of software) will
> want to use it.  One huge downside I see of making this part of
> systemd is Ubuntu.  They've decided not to use systemd for some
> unfathomable reason.

After some thought, I've decided that extending systemd is not the way
to go.  And the dbus API is really an overkill.  Writing a simple
system-wide daemon shouldn't be a challenge; the hard part is getting
git to use it properly.

^ permalink raw reply

* Re: inotify to minimize stat() calls
From: Junio C Hamano @ 2013-02-09 19:35 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Robert Zeh, Git List, Duy Nguyen
In-Reply-To: <CALkWK0mttn6E+D-22UBbvDCuNEy_jNOtBaKPS-a8mTbO2uAF3g@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> This is much better than Junio's suggestion to study possible
> implementations on all platforms and designing a generic daemon/
> communication channel.  That's no weekend project.

It appears that you misunderstood what I wrote.  That was not "here
is a design; I want it in my system.  Go implemment it".

It was "If somebody wants to discuss it but does not know where to
begin, doing a small experiment like this and reporting how well it
worked here may be one way to do so.", nothing more.

^ permalink raw reply

* [PATCH] git-bisect.txt: clarify that reset finishes bisect
From: Michael J Gruber @ 2013-02-09 19:47 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, Andreas Mohr

"reset" can be easily misunderstood as resetting a bisect session to its
start without finishing it. Clarify that it actually finishes the bisect
session.

Reported-by: Andreas Mohr <andi@lisas.de>
Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
 Documentation/git-bisect.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index b4831bb..d50bd89 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -83,7 +83,7 @@ Bisect reset
 ~~~~~~~~~~~~
 
 After a bisect session, to clean up the bisection state and return to
-the original HEAD, issue the following command:
+the original HEAD (i.e., to finish bisect), issue the following command:
 
 ------------------------------------------------
 $ git bisect reset
-- 
1.8.1.2.752.g32d147e

^ permalink raw reply related

* Re: [RFC/PATCH] Introduce branch.<name>.pushremote
From: Junio C Hamano @ 2013-02-09 20:00 UTC (permalink / raw)
  To: Ramkumar Ramachandra; +Cc: Git List, Jonathan Nieder, Jeff King
In-Reply-To: <CALkWK0kp5SvbeTQgLRKWUAT_koSi0wyp6iPTC324iH8+D8oeJg@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>
>> In other words, does it make sense to read branch.$name.pushremote
>> for all the other irrelevant branches?
>>
>> In yet other words, perhaps adding pushremote_name to the branch
>> structure is unneeded, and you only need this single global
>> variable?
>
> Frankly, I'm unhappy with this global.  Setting a global here and
> subsequently reading it in pushremote_get() feels flaky.

Why unhappy?

This is expressed as a per-branch configuration, but in realitiy the
configuration on any branch that is not the current one does *not*
matter an iota.

An API call find_pushremote_for_branch() that takes a branch name
and returns the pushremote for that arbitrary branch has no use in
your designed feature.  find_pushremote() that tells you where a
lazy "git push" goes under the conditoin is the only thing you need
in your design.  You can still implement it without globals, e.g.

	struct bp { const char *branch; const char *remotename; };

        static int pushremote(const char *var, const char *val, void *cb)
	{
		struct bp *bp = cb;
	        const char *name, *key;
                int namelen;

		if (!parse_config_key(var, "branch", &name, &namelen, &key) &&
                    namelen == strlen(bp->branch) &&
                    !memcmp(name, bp->branch, name, namelen) &&
		    !strcmp(key, "remotepush")) {
			git_config_string(&bp->remotename, var, val);
		}
		return 0;
	}

	char *find_pushremote(void) {
		struct bp bp;

                bp.branch = get_current_branch();
                bp.remotename = NULL;
                git_config(pushremote, &bp);
                return bp.remotename;
	}


And if you want to take default.pushremote that changes a lazy "git
push" to go to somewhere other than "origin", you make the
pushremote() callback notice that variable, add one element
"defaultremote" to struct "bp" to record that value, and then make
find_pushremote() do

	return bp.remotename ? bp.remotename : bp.defaultremote;

Sometimes "global variable" feels disturbing because then we cannot
run more than one user at the same time.  One caller may want to
know the value for "foo" branch while another want it for "bar"
branch.

But there is no reason for anybody to want to know the value for the
variables branch.$name.pushremote for all defined $name's at the
same time, exactly because this matters only for the current branch,
no?
			

^ permalink raw reply

* Re: Please pull l10n updates for 1.8.2 round 1
From: Junio C Hamano @ 2013-02-09 21:44 UTC (permalink / raw)
  To: Jiang Xin
  Cc: Ralf Thielow, Thomas Rast, Wang Sheng, Peter Krefting,
	Tran Ngoc Quan, Git List
In-Reply-To: <CANYiYbHhGDFD3LZKGeRxkBF5s0TRHMvRBZY7c9T8zONiZGiNdA@mail.gmail.com>

Thanks.

^ permalink raw reply

* [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport
From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw)
  To: gitster; +Cc: git, Ben Walton
In-Reply-To: <1360446418-12280-1-git-send-email-bdwalton@gmail.com>

Neither %s or %z are portable strftime format specifiers.  There is no
need for %s in git-cvsimport as the supplied time is already in
seconds since the epoch.  For %z, use the function get_tz_offset
provided by Git.pm instead.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
 git-cvsimport.perl |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 0a31ebd..344f120 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -26,6 +26,7 @@ use IO::Socket;
 use IO::Pipe;
 use POSIX qw(strftime tzset dup2 ENOENT);
 use IPC::Open2;
+use Git qw(get_tz_offset);
 
 $SIG{'PIPE'}="IGNORE";
 set_timezone('UTC');
@@ -864,7 +865,9 @@ sub commit {
 	}
 
 	set_timezone($author_tz);
-	my $commit_date = strftime("%s %z", localtime($date));
+	# $date is in the seconds since epoch format
+	my $tz_offset = get_tz_offset($date);
+	my $commit_date = "$date $tz_offset";
 	set_timezone('UTC');
 	$ENV{GIT_AUTHOR_NAME} = $author_name;
 	$ENV{GIT_AUTHOR_EMAIL} = $author_email;
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset
From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw)
  To: gitster; +Cc: git, Ben Walton
In-Reply-To: <1360446418-12280-1-git-send-email-bdwalton@gmail.com>

This function has utility outside of the SVN module for any routine
that needs the equivalent of GNU strftime's %z formatting option.
Move it to the top-level Git.pm so that non-SVN modules don't need to
import the SVN module to use it.

The rename makes the purpose of the function clearer.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
 perl/Git.pm         |   23 +++++++++++++++++++++++
 perl/Git/SVN.pm     |   12 ++----------
 perl/Git/SVN/Log.pm |    8 ++++++--
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 931047c..5649bcc 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -59,6 +59,7 @@ require Exporter;
                 command_bidi_pipe command_close_bidi_pipe
                 version exec_path html_path hash_object git_cmd_try
                 remote_refs prompt
+                get_tz_offset
                 temp_acquire temp_release temp_reset temp_path);
 
 
@@ -102,6 +103,7 @@ use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
 use Fcntl qw(SEEK_SET SEEK_CUR);
+use Time::Local qw(timelocal);
 }
 
 
@@ -511,6 +513,27 @@ C<git --html-path>). Useful mostly only internally.
 
 sub html_path { command_oneline('--html-path') }
 
+
+=item get_tz_offset ( TIME )
+
+Return the time zone offset from GMT in the form +/-HHMM where HH is
+the number of hours from GMT and MM is the number of minutes.  This is
+the equivalent of what strftime("%z", ...) would provide on a GNU
+platform.
+
+If TIME is not supplied, the current local time is used.
+
+=cut
+
+sub get_tz_offset {
+	# some systmes don't handle or mishandle %z, so be creative.
+	my $t = shift || time;
+	my $gm = timelocal(gmtime($t));
+	my $sign = qw( + + - )[ $t <=> $gm ];
+	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
+}
+
+
 =item prompt ( PROMPT , ISPASSWORD  )
 
 Query user C<PROMPT> and return answer from user.
diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 490e330..0ebc68a 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -11,7 +11,6 @@ use Carp qw/croak/;
 use File::Path qw/mkpath/;
 use File::Copy qw/copy/;
 use IPC::Open3;
-use Time::Local;
 use Memoize;  # core since 5.8.0, Jul 2002
 use Memoize::Storable;
 use POSIX qw(:signal_h);
@@ -22,6 +21,7 @@ use Git qw(
     command_noisy
     command_output_pipe
     command_close_pipe
+    get_tz_offset
 );
 use Git::SVN::Utils qw(
 	fatal
@@ -1311,14 +1311,6 @@ sub get_untracked {
 	\@out;
 }
 
-sub get_tz {
-	# some systmes don't handle or mishandle %z, so be creative.
-	my $t = shift || time;
-	my $gm = timelocal(gmtime($t));
-	my $sign = qw( + + - )[ $t <=> $gm ];
-	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
-}
-
 # parse_svn_date(DATE)
 # --------------------
 # Given a date (in UTC) from Subversion, return a string in the format
@@ -1351,7 +1343,7 @@ sub parse_svn_date {
 			delete $ENV{TZ};
 		}
 
-		my $our_TZ = get_tz();
+		my $our_TZ = get_tz_offset();
 
 		# This converts $epoch_in_UTC into our local timezone.
 		my ($sec, $min, $hour, $mday, $mon, $year,
diff --git a/perl/Git/SVN/Log.pm b/perl/Git/SVN/Log.pm
index 3cc1c6f..3f8350a 100644
--- a/perl/Git/SVN/Log.pm
+++ b/perl/Git/SVN/Log.pm
@@ -2,7 +2,11 @@ package Git::SVN::Log;
 use strict;
 use warnings;
 use Git::SVN::Utils qw(fatal);
-use Git qw(command command_oneline command_output_pipe command_close_pipe);
+use Git qw(command
+           command_oneline
+           command_output_pipe
+           command_close_pipe
+           get_tz_offset);
 use POSIX qw/strftime/;
 use constant commit_log_separator => ('-' x 72) . "\n";
 use vars qw/$TZ $limit $color $pager $non_recursive $verbose $oneline
@@ -119,7 +123,7 @@ sub run_pager {
 sub format_svn_date {
 	my $t = shift || time;
 	require Git::SVN;
-	my $gmoff = Git::SVN::get_tz($t);
+	my $gmoff = get_tz_offset($t);
 	return strftime("%Y-%m-%d %H:%M:%S $gmoff (%a, %d %b %Y)", localtime($t));
 }
 
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases
From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw)
  To: gitster; +Cc: git, Ben Walton
In-Reply-To: <1360446418-12280-1-git-send-email-bdwalton@gmail.com>

When passed a local time that was on the boundary of a DST change,
get_tz_offset returned a GMT offset that was incorrect (off by one
hour).  This is because the time was converted to GMT and then back to
a time stamp via timelocal() which cannot disambiguate boundary cases
as noted in its documentation.

Modify this algorithm, using an approach suggested by Junio C Hamano
that obtains the GMT time stamp by using timegm(localtime()) instead
of timelocal(gmtime()).  This avoids the ambigious conversion and
allows a correct time to be returned on every occassion.

Signed-off-by: Ben Walton <bdwalton@gmail.com>
---
 perl/Git.pm |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/perl/Git.pm b/perl/Git.pm
index 5649bcc..a56d1e7 100644
--- a/perl/Git.pm
+++ b/perl/Git.pm
@@ -103,7 +103,7 @@ use Error qw(:try);
 use Cwd qw(abs_path cwd);
 use IPC::Open2 qw(open2);
 use Fcntl qw(SEEK_SET SEEK_CUR);
-use Time::Local qw(timelocal);
+use Time::Local qw(timegm);
 }
 
 
@@ -528,8 +528,8 @@ If TIME is not supplied, the current local time is used.
 sub get_tz_offset {
 	# some systmes don't handle or mishandle %z, so be creative.
 	my $t = shift || time;
-	my $gm = timelocal(gmtime($t));
-	my $sign = qw( + + - )[ $t <=> $gm ];
+	my $gm = timegm(localtime($t));
+	my $sign = qw( + + - )[ $gm <=> $t ];
 	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
 }
 
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH 0/3] Fix a portability issue with git-cvsimport
From: Ben Walton @ 2013-02-09 21:46 UTC (permalink / raw)
  To: gitster; +Cc: git, Ben Walton

This is my (long overdue) re-roll of the series that fixes a
portability issue with git-cvsimport's use of strftime.  It also fixes
a but in the original implementation of get_tz (now get_tz_offset).

I ended up taking taking only part of the implementation suggested by
Junio.

The only usage of get_tz_offset is by git-cvsimport and Git::SVN::Log
currently.  There are tests that validate it works currently so I
didn't add anything additional.  If the git-cvsimport tests are
removed, there are no tests remaining that exercise the code full as
the SVN tests use UTC times.

Ben Walton (3):
  Move Git::SVN::get_tz to Git::get_tz_offset
  Fix get_tz_offset to properly handle DST boundary cases
  Avoid non-portable strftime format specifiers in git-cvsimport

 git-cvsimport.perl  |    5 ++++-
 perl/Git.pm         |   23 +++++++++++++++++++++++
 perl/Git/SVN.pm     |   12 ++----------
 perl/Git/SVN/Log.pm |    8 ++++++--
 4 files changed, 35 insertions(+), 13 deletions(-)

-- 
1.7.10.4

^ permalink raw reply

* Re: [PATCH] git-bisect.txt: clarify that reset finishes bisect
From: Junio C Hamano @ 2013-02-09 21:53 UTC (permalink / raw)
  To: Michael J Gruber; +Cc: git, Andreas Mohr
In-Reply-To: <5e23d4c420f150b700dd5100bffb38d32f874200.1360439176.git.git@drmicha.warpmail.net>

Michael J Gruber <git@drmicha.warpmail.net> writes:

> "reset" can be easily misunderstood as resetting a bisect session to its
> start without finishing it. Clarify that it actually finishes the bisect
> session.
>
> Reported-by: Andreas Mohr <andi@lisas.de>
> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
> ---
>  Documentation/git-bisect.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
> index b4831bb..d50bd89 100644
> --- a/Documentation/git-bisect.txt
> +++ b/Documentation/git-bisect.txt
> @@ -83,7 +83,7 @@ Bisect reset
>  ~~~~~~~~~~~~
>  
>  After a bisect session, to clean up the bisection state and return to
> -the original HEAD, issue the following command:
> +the original HEAD (i.e., to finish bisect), issue the following command:

Makes sense.

^ permalink raw reply

* Re: [PATCH 3/3] Avoid non-portable strftime format specifiers in git-cvsimport
From: Junio C Hamano @ 2013-02-09 22:20 UTC (permalink / raw)
  To: Ben Walton; +Cc: git
In-Reply-To: <1360446418-12280-4-git-send-email-bdwalton@gmail.com>

Ben Walton <bdwalton@gmail.com> writes:

> Neither %s or %z are portable strftime format specifiers.

Well, at least %z is in POSIX; "Some implementations of strftime(3)
lack support for %z format" is fine, tough.

Thanks.

^ permalink raw reply

* Re: [PATCH 2/3] Fix get_tz_offset to properly handle DST boundary cases
From: Junio C Hamano @ 2013-02-09 22:58 UTC (permalink / raw)
  To: Ben Walton; +Cc: git
In-Reply-To: <1360446418-12280-3-git-send-email-bdwalton@gmail.com>

Ben Walton <bdwalton@gmail.com> writes:

> When passed a local time that was on the boundary of a DST change,
> get_tz_offset returned a GMT offset that was incorrect (off by one
> hour).  This is because the time was converted to GMT and then back to
> a time stamp via timelocal() which cannot disambiguate boundary cases
> as noted in its documentation.
>
> Modify this algorithm, using an approach suggested by Junio C Hamano
> that obtains the GMT time stamp by using timegm(localtime()) instead
> of timelocal(gmtime()).  This avoids the ambigious conversion and
> allows a correct time to be returned on every occassion.

I'll reword the log message a bit to explain why the updated logic
is right and also refer to the message that has the suggestion.  o

The implemmentation is a bit dense to my taste, but looks correct (I
had to think about the comparison to come up with the value of the
$sign, though).

Thanks.

> Signed-off-by: Ben Walton <bdwalton@gmail.com>
> ---
>  perl/Git.pm |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/perl/Git.pm b/perl/Git.pm
> index 5649bcc..a56d1e7 100644
> --- a/perl/Git.pm
> +++ b/perl/Git.pm
> @@ -103,7 +103,7 @@ use Error qw(:try);
>  use Cwd qw(abs_path cwd);
>  use IPC::Open2 qw(open2);
>  use Fcntl qw(SEEK_SET SEEK_CUR);
> -use Time::Local qw(timelocal);
> +use Time::Local qw(timegm);
>  }
>  
>  
> @@ -528,8 +528,8 @@ If TIME is not supplied, the current local time is used.
>  sub get_tz_offset {
>  	# some systmes don't handle or mishandle %z, so be creative.
>  	my $t = shift || time;
> -	my $gm = timelocal(gmtime($t));
> -	my $sign = qw( + + - )[ $t <=> $gm ];
> +	my $gm = timegm(localtime($t));
> +	my $sign = qw( + + - )[ $gm <=> $t ];
>  	return sprintf("%s%02d%02d", $sign, (gmtime(abs($t - $gm)))[2,1]);
>  }

^ permalink raw reply

* Re: Feature request: Allow extracting revisions into directories
From: Junio C Hamano @ 2013-02-09 23:00 UTC (permalink / raw)
  To: Robert Clausecker; +Cc: git, Phil Hord
In-Reply-To: <1360425499.3369.10.camel@t520>

Robert Clausecker <fuzxxl@gmail.com> writes:

> After thinking a while about how to solve the problems I have, I
> consider the following things as a solution to my problem.
>
> Add an option --isolated, -i to git checkout: Check out a branch / tag /
> revision but do not touch the index. This could be used together with
> --work-tree to check out a branch into an arbitrary directory. Also, it
> satisfies all 4 criteria from [1] and therefore is perfect for
> deployment from a bare repository.
>
> What do you think about this feature request?

I am not Phil, but if you ask me, I think it is borderline between
"meh" and "no way we would give a short-and-sweet -i to something
like this".

^ permalink raw reply


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