Re: Is there a way to have local changes in a branch 'bake' while working in different branches?

2016-12-16 Thread John Keeping 
On Thu, Dec 15, 2016 at 08:14:58PM +, Larry Minton wrote:
> My question:
> 
> Let's say I have a code change that I want to 'bake' for a while
> locally, just to make sure some edge case doesn't pop up while I am
> working on other things.  Is there any practical way of doing that?  I
> could constantly merge that 'bake me' branch into other branches as I
> work on them and then remove those changes from the branches before
> sending them out for code review, but sooner or later pretty much
> guaranteed to screw that up

I wrote a tool [1] a while ago to manage integration branches so I use a
personal integration branch to pull together various in-progress
branches.

It means you can keep each topic in its own branch but work/test on top
of a unified branch by running:

git integration --rebuild my-integration-branch

whenever you change one of the topic branches.

I also use the instruction sheet to keep track of abandoned topics that
I might want to go back to but which are currently in a broken state,
you can see an example of that in my CGit integration branch [2].


[1] http://johnkeeping.github.io/git-integration/
[2] 
https://github.com/johnkeeping/cgit/blob/d01ce31ed3dfa9b05ef971464da2af5b9d6f2756/GIT-INTEGRATION-INSN

Re: Re: Homebrew and Git

2016-09-20 Thread John Keeping 
On Tue, Sep 20, 2016 at 01:07:00PM +0200, Heiko Voigt wrote:
> On Tue, Sep 20, 2016 at 01:02:28PM +0200, Heiko Voigt wrote:
> > Hi,
> > 
> > On Sun, Sep 18, 2016 at 05:50:28PM +0200, Jonas Thiel wrote:
> > > A while ago I have described my problem with Homebrew at the following
> > > GitHub channel
> > > (https://github.com/Homebrew/homebrew-core/issues/2970). In the
> > > meanwhile, I believe that I my problem with Homebrew is based on an
> > > issues with my Git. I have found the attached Git Crash reports on my
> > > Mac and because I am not familiar with reading/analysing Crash
> > > Reports, it would be great if someone could give me some feedback on
> > > it.
> > >  
> > > If you have any question, please do not hesitate to contact me.
> > 
> > From your crash reports I see that git is apparently crashing in a
> > strchr() call from within ident_default_email() which is a function that
> > tries to assemble a name and email to put into your commits.
> 
> BTW, here is the callstack inlined from the crashreport:
> 
> bsystem_platform.dylib0x7fff840db41c 
> _platform_strchr$VARIANT$Haswell + 28
> 1   git   0x00010ba1d3f4 ident_default_email 
> + 801
> 2   git   0x00010ba1d68f fmt_ident + 66
> 3   git   0x00010ba4b495 files_log_ref_write 
> + 175
> 4   git   0x00010ba4b0a6 commit_ref_update + 
> 106
> 5   git   0x00010ba4c3a8 
> ref_transaction_commit + 468
> 6   git   0x00010b994dd8 s_update_ref + 271
> 7   git   0x00010b994556 fetch_refs + 1969
> 8   git   0x00010b9935f2 fetch_one + 1913
> 9   git   0x00010b992bc4 cmd_fetch + 549
> 10  git   0x00010b9666c4 handle_builtin + 478
> 11  git   0x00010b96602f main + 376
> 12  libdyld.dylib 0x7fff834ef5ad start + 1
> 
> Maybe someone else has an idea what might be causing this...

The only strchr I can see that could be called here is in
canonical_name(), where it's called with addrinfo::ai_canonname.

Searching for OS X and ai_canonname, leads me straight back to this
list, although 7 years ago!  I think ident.c needs a fix similar to
commit 3e8a00a (daemon.c: fix segfault on OS X, 2009-04-27); from the
commit message there:

On OS X (and maybe other unices), getaddrinfo(3) returns NULL
in the ai_canonname field if it's called with an IP address for
the hostname.

Re: [PATCH v3] help: make option --help open man pages only for Git commands

2016-08-16 Thread John Keeping 
On Tue, Aug 16, 2016 at 06:20:30PM +0200, Ralf Thielow wrote:
> If option --help is passed to a Git command, we try to open
> the man page of that command.  However, we do it even for commands
> we don't know.  Make sure it is a Git command by using "help_unknown_cmd"
> which is even able to assume a command if the user made a typo.
> 
> This breaks "git  --help" while "git help " still works.
> 
> As " --help" will internally be turned into "help ",
> introduce the hidden option "--swapped" in order to know which
> version has been called.
> 
> Signed-off-by: Ralf Thielow 
> ---
> Thanks, all, for the help!
> 
> Changes since v2:
> - don't check for common guides as the list is very incomplete
> - only check for git commands when called via  --help (introduce
>   option --swapped for that), as suggested by Junio
> - change test case to check for --help being passed to a concept
>   used as a git command
> 
>  builtin/help.c  | 30 +++---
>  git.c   | 15 ++-
>  t/t0012-help.sh | 15 +++
>  3 files changed, 52 insertions(+), 8 deletions(-)
>  create mode 100755 t/t0012-help.sh
> 
> diff --git a/builtin/help.c b/builtin/help.c
> index 8848013..76f07c7 100644
> --- a/builtin/help.c
> +++ b/builtin/help.c
> @@ -37,7 +37,9 @@ static int show_all = 0;
>  static int show_guides = 0;
>  static unsigned int colopts;
>  static enum help_format help_format = HELP_FORMAT_NONE;
> +static int swapped = 0;
>  static struct option builtin_help_options[] = {
> + OPT_BOOL('s', "swapped", , "mark as being called by  
> --help"),

OPT_HIDDEN_BOOL maybe?

>   OPT_BOOL('a', "all", _all, N_("print all available commands")),
>   OPT_BOOL('g', "guides", _guides, N_("print list of useful 
> guides")),
>   OPT_SET_INT('m', "man", _format, N_("show man page"), 
> HELP_FORMAT_MAN),
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] help: make option --help open man pages only for Git commands

2016-08-16 Thread John Keeping 
On Mon, Aug 15, 2016 at 09:40:54PM +0100, Philip Oakley wrote:
> From: "Junio C Hamano" 
> > "Philip Oakley"  writes:
> >
> >> I'm still not sure this is enough. One of the problems back when I
> >> introduced the --guides option (65f9835 (builtin/help.c: add --guide
> >> option, 2013-04-02)) was that we had no easy way of determining what
> >> guides were available, especially given the *nix/Windows split where
> >> the help defaults are different (--man/--html).
> >>
> >> At the time[1] we (I) punted on trying to determine which guides were
> >> actually installed, and just created a short list of the important
> >> guides, which I believe you now check. However the less common guides
> >> are still there (gitcvs-migration?), and others may be added locally.
> >
> > I think we should do both; "git help cvs-migration" should keep the
> > same codeflow and behaviour as we have today (so that it would still
> > work), while "git cvs-migration --help" should say "'cvs-migration'
> > is not a git command".  That would be a good clean-up anyway.
> >
> > It obviously cannot be done if git.c::handle_builtin() does the same
> > "swap  --help to help " hack, but we could improve that
> > part (e.g. rewrite it to "help --swapped " to allow cmd_help()
> > to notice).  When the user said " --help", we don't do guides,
> > when we swapped the word order, we check with guides, too.
> >
> The other option is to simply build a guide-list in exactly the same format 
> as the command list (which if it works can be merged later). Re-use the 
> existing code, etc.

One nice thing at the moment is that third-party Git commands can
install documentation and have "git help" work correctly (shameless plug
for git-integration[1] which does this).  I think Junio's suggestion
above keeps that working whereas having a hardcoded list of guides will
break this.

[1] https://github.com/johnkeeping/git-integration
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] difftool: always honor fatal error exit codes

2016-08-15 Thread John Keeping 
At the moment difftool's "trust exit code" logic always suppresses the
exit status of the diff utility we invoke.  This is useful because we
don't want to exit just because diff returned "1" because the files
differ, but it's confusing if the shell returns an error because the
selected diff utility is not found.

POSIX specifies 127 as the exit status for "command not found", 126 for
"command found but is not executable" and values greater than 128 if the
command terminated because it received a signal [1] and at least bash
and dash follow this specification, while diff utilities generally use
"1" for the exit status we want to ignore.

Handle any value of 126 or greater as a special value indicating that
some form of fatal error occurred.

[1] 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
On Mon, Aug 15, 2016 at 10:35:26PM +0100, John Keeping wrote:
> On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> > "Tom Tanner (BLOOMBERG/ LONDON)" <ttann...@bloomberg.net> writes:
> > 
> > > From: gits...@pobox.com
> > > To: j...@keeping.me.uk
> > > Cc: Tom Tanner (BLOOMBERG/ LONDON), dav...@gmail.com, git@vger.kernel.org
> > > At: 08/14/16 04:21:18
> > >
> > > John Keeping <j...@keeping.me.uk> writes:
> > > ...
> > >> POSIX specifies 127 as the exit status for "command not found" and 126
> > >> for "command found but is not executable" [1] and at least bash and dash
> > >> follow this specification, while diff utilities generally use "1" for
> > >> the exit status we want to ignore.
> > >>
> > >> Handle 126 and 127 as special values, assuming that they always mean
> > >> that the command could not be executed.
> > >
> > > Sounds like a reasonable thing to do.  Will queue; thanks.
> > 
> > > Would it be possible to also treat signals (128 and above) as
> > > 'special' values as well (as I've seen some merge tools self
> > > destruct like that from time to time)
> > 
> > Certainly, it feels safer to notice an unusual exit status code and
> > error out to force the user to take notice, but that reasoning
> > assumes that "128 and above" are noteworthy exceptions.
> 
> Reading further in POSIX:
> 
>   The exit status of a command that terminated because it received
>   a signal shall be reported as greater than 128.
> 
> I think if we accept the argument above about diff utilities generally
> using low numbers for the status values we're ignoring intentionally,
> then we can just treat any value above 125 as a fatal error.

Here's what that looks like.

 git-difftool--helper.sh | 7 +++
 t/t7800-difftool.sh | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 84d6cc0..7bfb673 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -86,6 +86,13 @@ else
do
launch_merge_tool "$1" "$2" "$5"
status=$?
+   if test $status -ge 126
+   then
+   # Command not found (127), not executable (126) or
+   # exited via a signal (>= 128).
+   exit $status
+   fi
+
if test "$status" != 0 &&
test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
then
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2974900..70a2de4 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with 
--trust-exit-code' '
test_cmp expect actual
 '
 
+test_expect_success PERL 'difftool honors exit status if command not found' '
+   test_config difftool.nonexistent.cmd i-dont-exist &&
+   test_config difftool.trustExitCode false &&
+   test_must_fail git difftool -y -t nonexistent branch
+'
+
 test_expect_success PERL 'difftool honors --gui' '
difftool_test_setup &&
test_config merge.tool bogus-tool &&
-- 
2.9.3.728.g30b24b4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool: always honor "command not found" exit code

2016-08-15 Thread John Keeping 
On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> "Tom Tanner (BLOOMBERG/ LONDON)" <ttann...@bloomberg.net> writes:
> 
> > From: gits...@pobox.com
> > To: j...@keeping.me.uk
> > Cc: Tom Tanner (BLOOMBERG/ LONDON), dav...@gmail.com, git@vger.kernel.org
> > At: 08/14/16 04:21:18
> >
> > John Keeping <j...@keeping.me.uk> writes:
> > ...
> >> POSIX specifies 127 as the exit status for "command not found" and 126
> >> for "command found but is not executable" [1] and at least bash and dash
> >> follow this specification, while diff utilities generally use "1" for
> >> the exit status we want to ignore.
> >>
> >> Handle 126 and 127 as special values, assuming that they always mean
> >> that the command could not be executed.
> >
> > Sounds like a reasonable thing to do.  Will queue; thanks.
> 
> > Would it be possible to also treat signals (128 and above) as
> > 'special' values as well (as I've seen some merge tools self
> > destruct like that from time to time)
> 
> Certainly, it feels safer to notice an unusual exit status code and
> error out to force the user to take notice, but that reasoning
> assumes that "128 and above" are noteworthy exceptions.

Reading further in POSIX:

The exit status of a command that terminated because it received
a signal shall be reported as greater than 128.

I think if we accept the argument above about diff utilities generally
using low numbers for the status values we're ignoring intentionally,
then we can just treat any value above 125 as a fatal error.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

2016-08-15 Thread John Keeping 
On Mon, Aug 15, 2016 at 08:30:03PM +0700, Duy Nguyen wrote:
> On Mon, Aug 15, 2016 at 7:37 PM, Philip Oakley  wrote:
> > I appreciate there has been a lot of discussion, but it mainly appears to be
> > about an upstream / integration viewpoint.
> >
> > I'd hate it if there was a one size fits all solution that was only focused
> > on one important use case, rather than having at least a simple fallback for
> > simple folk.
> >
> > Personally I liked the idea that I could start my patch series branch with a
> > simple 'empty' commit with a commit message that read "cover!  > the series>" and continue with the cover letter. It's essentially the same
> > as the fixup! and squash! idea (more the latter - it's squash! without a
> > predecessor). For moderate size series a simple 'git rebase master..' is
> > sufficient to see the whole series and decide which need editing, rewording,
> > swapping, checking the fixups, etc.
> 
> I think you hit the jackpot (or are getting very close). This removes
> the special status of "the commit at the tip of the branch" cover
> letter. Maybe I just like it so much I have a hard time finding
> anything wrong with it :)

I haven't followed this thread too closely, but has anyone mentioned
U-Boot's patman tool[1] yet?

It defines several special trailers that can be used to annotate commits
with additional information to use when mailing them and which are
automatically removed from the commit message in patches sent using
patman.


[1] http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

2016-08-14 Thread John Keeping 
On Sat, Aug 13, 2016 at 08:42:21PM -0700, David Aguilar wrote:
> This use case makes me wonder whether the sorting we do here is
> something that should be opened up a bit so that the it's not
> quite so set in stone.
> 
> For example, an extension to the approach taken by this patch
> would be to have `mergetool.reverseOrder` git config boolean
> option that would tell us whether or not to use the "-r" flag
> when calling sort.
> 
> But, IMO that is too rigid, and only addresses this narrow use
> case.  What if users want a case-insensitive sort, or some other
> preferred ordering?
> 
> We can address these concerns, and your use case, by opening it
> up. Something like,
> 
>   sort=$(git config mergetool.sort || echo sort -u)

Why not reuse the existing diff.orderFile config variable?  (Also
supported by the -O option to git-diff).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] difftool: always honor "command not found" exit code

2016-08-13 Thread John Keeping 
At the moment difftool's "trust exit code" logic always suppresses the
exit status of the diff utility we invoke.  This is useful because we
don't want to exit just because diff returned "1" because the files
differ, but it's confusing if the shell returns an error because the
selected diff utility is not found.

POSIX specifies 127 as the exit status for "command not found" and 126
for "command found but is not executable" [1] and at least bash and dash
follow this specification, while diff utilities generally use "1" for
the exit status we want to ignore.

Handle 126 and 127 as special values, assuming that they always mean
that the command could not be executed.

[1] 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
On Sat, Aug 13, 2016 at 11:36:39AM +0100, John Keeping wrote:
> It would be nice if there was a way to differentiate between complete
> failure and just the diff tool exiting with a non-zero return status
> because the files differ, but I'm not sure whether we can do that
> reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
> run [1] so it may be sensible to to treat those as special values.

Something like this perhaps?  I think this is probably safe, but it's
always possible that some diff utility does use 126 or 127 as a "normal"
exit status.  I'm not sure what we can do about that other than add a
"really, really don't trust the exit status" option!

 git-difftool--helper.sh | 18 ++
 t/t7800-difftool.sh |  6 ++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 84d6cc0..68877d4 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -86,11 +86,21 @@ else
do
launch_merge_tool "$1" "$2" "$5"
status=$?
-   if test "$status" != 0 &&
-   test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
-   then
+   case "$status" in
+   0)
+   : OK
+   ;;
+   126|127)
+   # Command not found or not executable
exit $status
-   fi
+   ;;
+   *)
+   if test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
+   then
+   exit $status
+   fi
+   ;;
+   esac
shift 7
done
 fi
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2974900..70a2de4 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with 
--trust-exit-code' '
test_cmp expect actual
 '
 
+test_expect_success PERL 'difftool honors exit status if command not found' '
+   test_config difftool.nonexistent.cmd i-dont-exist &&
+   test_config difftool.trustExitCode false &&
+   test_must_fail git difftool -y -t nonexistent branch
+'
+
 test_expect_success PERL 'difftool honors --gui' '
difftool_test_setup &&
test_config merge.tool bogus-tool &&
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git difftool and git mergetool aren't returning errors when the tool has issues

2016-08-13 Thread John Keeping 
On Fri, Aug 12, 2016 at 07:13:41AM -, Tom Tanner (BLOOMBERG/ LONDON) wrote:
> For instance, if you set your diff/mergetool to meld and you don't have it 
> installed:
> > git difftool
> 
> Viewing (1/1): 'blah'
> Launch 'meld' [Y/n]? y
> /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> > echo $?
> 0
> 
> > /home/ttanner/bin/meld
> /home/ttanner/bin/meld[8]: /opt/swt/bin/meld: not found
> > echo $?
> 127

Have you looked at the --trust-exit-code option to git-difftool?

It would be nice if there was a way to differentiate between complete
failure and just the diff tool exiting with a non-zero return status
because the files differ, but I'm not sure whether we can do that
reliably.  POSIX uses 127 and 126 as errors that mean the command didn't
run [1] so it may be sensible to to treat those as special values.

[1] 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

2016-08-07 Thread John Keeping 
On Sun, Aug 07, 2016 at 08:12:23AM +0300, Michael S. Tsirkin wrote:
> On Fri, Aug 05, 2016 at 08:39:58AM -0700, Junio C Hamano wrote:
> >  * When you updated an existing topic, you tell a tool like "rebase
> >-i -p" to recreate "lit" branch on top of the mainline.  This
> >would give you an opportunity to update the cover.
> 
> Combining patchsets might need conflict resolution,
> redoing this each time might be a lot of work.

git-rerere can generally handle that pretty well.  I wrote a tool [1] to
manage integration branches which I use pretty heavily and I find it
very rare to hit a serious conflict.  In fact, git-integration has an
"autocontinue" mode which accepts git-rerere's resolution if it has one,
which works reliably in my experience.

I hadn't thought about writing the cover letter in the integration
branch instruction sheet (I normally just put in some notes for myself
about the state of the branch), but I suspect it would be quite easy to
write a script that mails a series using the instruction sheet comments
as the cover letter.

[1] http://johnkeeping.github.io/git-integration/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Aug 2016, #01; Tue, 2)

2016-08-04 Thread John Keeping 
On Thu, Aug 04, 2016 at 10:03:39AM +0200, Lars Schneider wrote:
> 
> > 
> > * jk/push-force-with-lease-creation (2016-07-26) 3 commits
> > - push: allow pushing new branches with --force-with-lease
> > - push: add shorthand for --force-with-lease branch creation
> > - Documentation/git-push: fix placeholder formatting
> > 
> > "git push --force-with-lease" already had enough logic to allow
> > ensuring that such a push results in creation of a ref (i.e. the
> > receiving end did not have another push from sideways that would be
> > discarded by our force-pushing), but didn't expose this possibility
> > to the users.  It does so now.
> > 
> > Will merge to 'next'.
> 
> t5533-push-cas.sh "16 - new branch already exists" seems to be broken 
> for OSX on next. Git bisect indicates that "push: add shorthand for 
> --force-with-lease branch creation" might be the culprit.
> 
> https://travis-ci.org/git/git/jobs/149614431
> https://api.travis-ci.org/jobs/149614431/log.txt?deansi=true (non-JS)

It seems that the test script has already done "test_commit C", so the
newly added "test_commit c" does nothing on a case-insensitive
filesystem.

Something like this will make the test more consistent with the rest of
the file:

diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 5f29664..e5bbbd8 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -220,7 +220,7 @@ test_expect_success 'new branch already exists' '
(
cd src &&
git checkout -b branch master &&
-   test_commit c
+   test_commit F
) &&
(
cd dst &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/3] push: allow pushing new branches with --force-with-lease

2016-07-26 Thread John Keeping 
Changes in v3:
- Use hashclr() and oidclr() where appropriate instead of memset()
- Pull a test forward from patch 3 to patch 2 

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c   |  9 +
 remote.h   |  1 -
 t/t5533-push-cas.sh| 38 ++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 1/3] Documentation/git-push: fix placeholder formatting

2016-07-26 Thread John Keeping 
Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
No changes in v3.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=:` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value  (which is allowed to be
+the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 3/3] push: allow pushing new branches with --force-with-lease

2016-07-26 Thread John Keeping 
If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=: syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
Changes in v3:
- use oidclr()
- final test is now added in the previous patch and now uses the
  explicit --force-with-lease syntax

 remote.c|  7 +++
 remote.h|  1 -
 t/t5533-push-cas.sh | 12 
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index 42c4a34..d29850a 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * branch.
 */
if (ref->expect_old_sha1) {
-   if (ref->expect_old_no_trackback ||
-   oidcmp(>old_oid, >old_oid_expect))
+   if (oidcmp(>old_oid, >old_oid_expect))
reject_reason = REF_STATUS_REJECT_STALE;
else
/* If the ref isn't stale then force the 
update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(>old_oid_expect);
return;
}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(>old_oid_expect);
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   expect_old_no_trackback:1,
deletion:1,
matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index ed631c3..09899af 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
setup_srcdst_basic &&
(
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 2/3] push: add shorthand for --force-with-lease branch creation

2016-07-26 Thread John Keeping 
Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
Changes in v3:
- use hashclr()
- pull 'new branch already exists' test forward from patch 3 and use
  explicit --force-with-lease syntax

 Documentation/git-push.txt |  3 ++-
 remote.c   |  2 ++
 t/t5533-push-cas.sh| 26 ++
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
value to be
 the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=:`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..42c4a34 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
entry = add_cas_entry(cas, arg, colon - arg);
if (!*colon)
entry->use_tracking = 1;
+   else if (!colon[1])
+   hashclr(entry->expect);
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..ed631c3 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,30 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'new branch already exists' '
+   setup_srcdst_basic &&
+   (
+   cd src &&
+   git checkout -b branch master &&
+   test_commit c
+   ) &&
+   (
+   cd dst &&
+   git branch branch master &&
+   test_must_fail git push --force-with-lease=branch: origin branch
+   )
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

2016-07-26 Thread John Keeping 
On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> >> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option 
> >> > *cas, const char *arg, int unse
> >> >  entry = add_cas_entry(cas, arg, colon - arg);
> >> >  if (!*colon)
> >> >  entry->use_tracking = 1;
> >> > +else if (!colon[1])
> >> > +memset(entry->expect, 0, sizeof(entry->expect));
> >> 
> >> hashclr()?
> >
> > Yes (and in the following patch as well).  I hadn't realised that
> > function exists.
> 
> Thanks; I've locally tweaked these two patches; the interdiff looks
> like this.

Thanks.  I'm about to send v3 anyway to pull a test forward to address
Jakub's comment.  I also used oidclr() for the last two changes below.

>  remote.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/remote.c b/remote.c
> index e8b7bac..7eaf3c8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -2304,7 +2304,7 @@ int parse_push_cas_option(struct push_cas_option *cas, 
> const char *arg, int unse
>   if (!*colon)
>   entry->use_tracking = 1;
>   else if (!colon[1])
> - memset(entry->expect, 0, sizeof(entry->expect));
> + hashclr(entry->expect);
>   else if (get_sha1(colon + 1, entry->expect))
>   return error("cannot parse expected object name '%s'", colon + 
> 1);
>   return 0;
> @@ -2354,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
>   if (!entry->use_tracking)
>   hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
>   else if (remote_tracking(remote, ref->name, 
> >old_oid_expect))
> - memset(>old_oid_expect, 0, 
> sizeof(ref->old_oid_expect));
> + hashclr(ref->old_oid_expect.hash);
>   return;
>   }
>  
> @@ -2364,7 +2364,7 @@ static void apply_cas(struct push_cas_option *cas,
>  
>   ref->expect_old_sha1 = 1;
>   if (remote_tracking(remote, ref->name, >old_oid_expect))
> - memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
> + hashclr(ref->old_oid_expect.hash);
>  }
>  
>  void apply_push_cas(struct push_cas_option *cas,
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

2016-07-26 Thread John Keeping 
On Tue, Jul 26, 2016 at 12:30:05PM +0200, Jakub Narębski wrote:
> W dniu 2016-07-25 o 23:59, John Keeping pisze:
> 
> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +   setup_srcdst_basic &&
> > +   (
> > +   cd dst &&
> > +   git branch branch master &&
> > +   git push --force-with-lease=branch: origin branch
> > +   ) &&
> > +   git ls-remote dst refs/heads/branch >expect &&
> > +   git ls-remote src refs/heads/branch >actual &&
> > +   test_cmp expect actual
> > +'
> 
> Do we need to test the negative, that is that if branch is not
> new it prevents push (e.g. when  is HEAD), or is it
> covered by other tests?

It's covered by a test in patch 3 (at least for the implicit case added
there), but I could pull that forwards.  In fact, converting that test
to the explicit syntax will make it simpler since we won't need to set
up a non-fast-forward push.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

2016-07-26 Thread John Keeping 
On Mon, Jul 25, 2016 at 03:22:48PM -0700, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> > Allow the empty string to stand in for the null SHA-1 when pushing a new
> > branch, like we do when deleting branches.
> >
> > This means that the following command ensures that `new-branch` is
> > created on the remote (that is, is must not already exist):
> >
> > git push --force-with-lease=new-branch: origin new-branch
> >
> > Signed-off-by: John Keeping <j...@keeping.me.uk>
> > ---
> > New in v2.
> >
> >  Documentation/git-push.txt |  3 ++-
> >  remote.c   |  2 ++
> >  t/t5533-push-cas.sh| 12 
> >  3 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
> > index bf7c9a2..927a034 100644
> > --- a/Documentation/git-push.txt
> > +++ b/Documentation/git-push.txt
> > @@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
> > value to be
> >  the same as the specified value `` (which is allowed to be
> >  different from the remote-tracking branch we have for the refname,
> >  or we do not even have to have such a remote-tracking branch when
> > -this form is used).
> > +this form is used).  If `` is the empty string, then the named ref
> > +must not already exist.
> >  +
> >  Note that all forms other than `--force-with-lease=:`
> >  that specifies the expected current value of the ref explicitly are
> > diff --git a/remote.c b/remote.c
> > index a326e4e..af94892 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option 
> > *cas, const char *arg, int unse
> > entry = add_cas_entry(cas, arg, colon - arg);
> > if (!*colon)
> > entry->use_tracking = 1;
> > +   else if (!colon[1])
> > +   memset(entry->expect, 0, sizeof(entry->expect));
> 
> hashclr()?

Yes (and in the following patch as well).  I hadn't realised that
function exists.

> > else if (get_sha1(colon + 1, entry->expect))
> > return error("cannot parse expected object name '%s'", colon + 
> > 1);
> > return 0;
> > diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> > index c732012..5e7f6e9 100755
> > --- a/t/t5533-push-cas.sh
> > +++ b/t/t5533-push-cas.sh
> > @@ -191,4 +191,16 @@ test_expect_success 'cover everything with default 
> > force-with-lease (allowed)' '
> > test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +   setup_srcdst_basic &&
> > +   (
> > +   cd dst &&
> > +   git branch branch master &&
> > +   git push --force-with-lease=branch: origin branch
> > +   ) &&
> > +   git ls-remote dst refs/heads/branch >expect &&
> > +   git ls-remote src refs/heads/branch >actual &&
> > +   test_cmp expect actual
> > +'
> > +
> >  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/3] push: allow pushing new branches with --force-with-lease

2016-07-25 Thread John Keeping 
On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> > If there is no upstream information for a branch, it is likely that it
> > is newly created and can safely be pushed under the normal fast-forward
> > rules.  Relax the --force-with-lease check so that we do not reject
> > these branches immediately but rather attempt to push them as new
> > branches, using the null SHA-1 as the expected value.
> >
> > In fact, it is already possible to push new branches using the explicit
> > --force-with-lease=: syntax, so all we do here is make
> > this behaviour the default if no explicit "expect" value is specified.
> 
> I like the loss of an extra field from "struct ref".
> 
> I suspect that the if/else cascade in the loop in apply_cas() can
> also be taught that ':' followed by an empty string asks to check
> that the target ref does not exist, in order to make it a bit more
> useful for folks who do not rely on the "use the last observed
> status of the tracking branch".
> 
> That would make the "explicit" test much less cumbersome to read.

Yes, that's nicer and it mirrors the syntax for deleting a remote
branch.

I've pulled it out as a preparatory step because I like the fact that
the "explicit" test passes even before the patch that is the main point
of the series.

> > +test_expect_success 'new branch covered by force-with-lease (explicit)' '
> > +   setup_srcdst_basic &&
> > +   (
> > +   cd dst &&
> > +   git branch branch master &&
> > +   git push 
> > --force-with-lease=branch: origin 
> > branch
> > +   ) &&

John Keeping (3):
  Documentation/git-push: fix placeholder formatting
  push: add shorthand for --force-with-lease branch creation
  push: allow pushing new branches with --force-with-lease

 Documentation/git-push.txt |  5 +++--
 remote.c   |  9 +
 remote.h   |  1 -
 t/t5533-push-cas.sh| 38 ++
 4 files changed, 46 insertions(+), 7 deletions(-)

-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/3] Documentation/git-push: fix placeholder formatting

2016-07-25 Thread John Keeping 
Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
New in v2.

 Documentation/git-push.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 93c3527..bf7c9a2 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -198,7 +198,7 @@ branch we have for it.
 +
 `--force-with-lease=:` will protect the named ref (alone),
 if it is going to be updated, by requiring its current value to be
-the same as the specified value  (which is allowed to be
+the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
 this form is used).
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/3] push: add shorthand for --force-with-lease branch creation

2016-07-25 Thread John Keeping 
Allow the empty string to stand in for the null SHA-1 when pushing a new
branch, like we do when deleting branches.

This means that the following command ensures that `new-branch` is
created on the remote (that is, is must not already exist):

git push --force-with-lease=new-branch: origin new-branch

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
New in v2.

 Documentation/git-push.txt |  3 ++-
 remote.c   |  2 ++
 t/t5533-push-cas.sh| 12 
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index bf7c9a2..927a034 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -201,7 +201,8 @@ if it is going to be updated, by requiring its current 
value to be
 the same as the specified value `` (which is allowed to be
 different from the remote-tracking branch we have for the refname,
 or we do not even have to have such a remote-tracking branch when
-this form is used).
+this form is used).  If `` is the empty string, then the named ref
+must not already exist.
 +
 Note that all forms other than `--force-with-lease=:`
 that specifies the expected current value of the ref explicitly are
diff --git a/remote.c b/remote.c
index a326e4e..af94892 100644
--- a/remote.c
+++ b/remote.c
@@ -2294,6 +2294,8 @@ int parse_push_cas_option(struct push_cas_option *cas, 
const char *arg, int unse
entry = add_cas_entry(cas, arg, colon - arg);
if (!*colon)
entry->use_tracking = 1;
+   else if (!colon[1])
+   memset(entry->expect, 0, sizeof(entry->expect));
else if (get_sha1(colon + 1, entry->expect))
return error("cannot parse expected object name '%s'", colon + 
1);
return 0;
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..5e7f6e9 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,16 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/3] push: allow pushing new branches with --force-with-lease

2016-07-25 Thread John Keeping 
If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=: syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
Changes in v2:
- The "explicit" test was previously in this patch but is now added in
  patch 2/3.

 remote.c|  7 +++
 remote.h|  1 -
 t/t5533-push-cas.sh | 26 ++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index af94892..20e174d 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * branch.
 */
if (ref->expect_old_sha1) {
-   if (ref->expect_old_no_trackback ||
-   oidcmp(>old_oid, >old_oid_expect))
+   if (oidcmp(>old_oid, >old_oid_expect))
reject_reason = REF_STATUS_REJECT_STALE;
else
/* If the ref isn't stale then force the 
update. */
@@ -2345,7 +2344,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, 
sizeof(ref->old_oid_expect));
return;
}
 
@@ -2355,7 +2354,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   expect_old_no_trackback:1,
deletion:1,
matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 5e7f6e9..5f29664 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,6 +191,18 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_expect_success 'new branch covered by force-with-lease (explicit)' '
setup_srcdst_basic &&
(
@@ -203,4 +215,18 @@ test_expect_success 'new branch covered by 
force-with-lease (explicit)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch already exists' '
+   setup_srcdst_basic &&
+   (
+   cd src &&
+   git checkout -b branch master &&
+   test_commit c
+   ) &&
+   (
+   cd dst &&
+   git branch branch master &&
+   test_must_fail git push --force-with-lease=branch origin branch
+   )
+'
+
 test_done
-- 
2.9.2.639.g855ae9f

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] push: allow pushing new branches with --force-with-lease

2016-07-23 Thread John Keeping 
If there is no upstream information for a branch, it is likely that it
is newly created and can safely be pushed under the normal fast-forward
rules.  Relax the --force-with-lease check so that we do not reject
these branches immediately but rather attempt to push them as new
branches, using the null SHA-1 as the expected value.

In fact, it is already possible to push new branches using the explicit
--force-with-lease=: syntax, so all we do here is make
this behaviour the default if no explicit "expect" value is specified.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 remote.c|  7 +++
 remote.h|  1 -
 t/t5533-push-cas.sh | 38 ++
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index a326e4e..cd2ee52 100644
--- a/remote.c
+++ b/remote.c
@@ -1544,8 +1544,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int 
send_mirror,
 * branch.
 */
if (ref->expect_old_sha1) {
-   if (ref->expect_old_no_trackback ||
-   oidcmp(>old_oid, >old_oid_expect))
+   if (oidcmp(>old_oid, >old_oid_expect))
reject_reason = REF_STATUS_REJECT_STALE;
else
/* If the ref isn't stale then force the 
update. */
@@ -2343,7 +2342,7 @@ static void apply_cas(struct push_cas_option *cas,
if (!entry->use_tracking)
hashcpy(ref->old_oid_expect.hash, cas->entry[i].expect);
else if (remote_tracking(remote, ref->name, 
>old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, 
sizeof(ref->old_oid_expect));
return;
}
 
@@ -2353,7 +2352,7 @@ static void apply_cas(struct push_cas_option *cas,
 
ref->expect_old_sha1 = 1;
if (remote_tracking(remote, ref->name, >old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(>old_oid_expect, 0, sizeof(ref->old_oid_expect));
 }
 
 void apply_push_cas(struct push_cas_option *cas,
diff --git a/remote.h b/remote.h
index c21fd37..9248811 100644
--- a/remote.h
+++ b/remote.h
@@ -89,7 +89,6 @@ struct ref {
force:1,
forced_update:1,
expect_old_sha1:1,
-   expect_old_no_trackback:1,
deletion:1,
matched:1;
 
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..4276b1b 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,42 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'new branch covered by force-with-lease (explicit)' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push 
--force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'new branch already exists' '
+   setup_srcdst_basic &&
+   (
+   cd src &&
+   git checkout -b branch master &&
+   test_commit c
+   ) &&
+   (
+   cd dst &&
+   git branch branch master &&
+   test_must_fail git push --force-with-lease=branch origin branch
+   )
+'
+
 test_done
-- 
2.9.2.637.g8b832fc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] difftool: fix argument handling in subdirs

2016-07-05 Thread John Keeping 
On Mon, Jul 04, 2016 at 08:37:39PM +0200, Bernhard Kirchen wrote:
> Today I started using --dir-diff and noticed a problem when specifying a
> non-full path limiter. My diff tool is setup to be meld (*1).
> 
> OK while working directory is repo root; also OK while working directory is
> repo subfolder "actual":
> git difftool --dir-diff HEAD~1 HEAD -- actual/existing/path
> => meld opens with proper dir-diff.
> 
> NOT OK while working directory is repo subfolder "actual":
> git difftool --dir-diff HEAD~1 HEAD -- existing/path
> => nothing happens, as if using "non/such/path" as the path limiter.
> 
> Because "git diff HEAD~1 HEAD -- existing/path" while the working directory
> is the repo subfolder "actual" works, I epxected the difftool to work
> similarly. Is this a bug?

I think it is, yes.  The patch below fixes it for me and doesn't break
any existing tests, but I still don't understand why the separate
$diffrepo was needed originally, so I'm not certain this won't break
some other corner case.

-- >8 --
When in a subdirectory of a repository, path arguments should be
interpreted relative to the current directory not the root of the
working tree.

The Git::repository object passed into setup_dir_diff() is configured to
handle this correctly but we create a new Git::repository here without
setting the WorkingSubdir argument.  By simply using the existing
repository, path arguments are handled relative to the current
directory.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 git-difftool.perl | 13 +++--
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index ebd13ba..c9d3ef8 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -115,16 +115,9 @@ sub setup_dir_diff
 {
my ($repo, $workdir, $symlinks) = @_;
 
-   # Run the diff; exit immediately if no diff found
-   # 'Repository' and 'WorkingCopy' must be explicitly set to insure that
-   # if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
-   # by Git->repository->command*.
my $repo_path = $repo->repo_path();
-   my %repo_args = (Repository => $repo_path, WorkingCopy => $workdir);
-   my $diffrepo = Git->repository(%repo_args);
-
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
-   my $diffrtn = $diffrepo->command_oneline(@gitargs);
+   my $diffrtn = $repo->command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
 
# Build index info for left and right sides of the diff
@@ -176,12 +169,12 @@ EOF
 
if ($lmode eq $symlink_mode) {
$symlink{$src_path}{left} =
-   $diffrepo->command_oneline('show', "$lsha1");
+   $repo->command_oneline('show', "$lsha1");
}
 
if ($rmode eq $symlink_mode) {
$symlink{$dst_path}{right} =
-   $diffrepo->command_oneline('show', "$rsha1");
+   $repo->command_oneline('show', "$rsha1");
}
 
if ($lmode ne $null_mode and $status !~ /^C/) {
-- 
2.9.0.465.g8850cbc

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] git-submodule has bash-ism?

2016-06-01 Thread John Keeping 
On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> > On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
> >> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
> >> 
> >> > > >  reset_submodule_urls () {
> >> > > > -local root
> >> > > > -root=$(pwd) &&
> >> > > >  (
> >> > > > +root=$(pwd) &&
> >> > > >  cd super-clone/submodule &&
> >> > > >  git config remote.origin.url "$root/submodule"
> >> > > >  ) &&
> >> > > >  (
> >> > > > +root=$(pwd) &&
> >> > > >  cd super-clone/submodule/sub-submodule &&
> >> > > >  git config remote.origin.url "$root/submodule"
> >> > [...]
> >> > I wonder if it's relevant that the "local root" line isn't &&-chained?
> >> > Is it possible that on some shells we ignore an error but everything
> >> > still works?
> >> 
> >> I don't think so. We're inside a function, so we wouldn't affect any
> >> outer &&-chaining in the function (and there isn't any in the caller
> >> anyway). I think it's a reasonable custom not to bother &&-chaining
> >> "local" lines, as they come at the top of a function and can't fail.
> >
> > Can't fail if the shell supports "local", but if we're in a shell that
> > doesn't support it, then the lack of "&&" may allow us to just carry on.
> 
> True, but if "to just carry on" were a correct behaviour, then
> wouldn't that mean that "local" was unnecessary, i.e. the variable
> did not have to get localized because stomping on the global name
> would not affect later reference to the same variable made by the
> caller?
> 
> If the clobbering of a global variable breaks the behaviour of the
> script, wouldn't we rather want to catch that fact?
> 
> So either way, I do not think "local variable names" that breaks
> &&-chain can be justified.  Either the variable must be localized
> for the script to work correctly, in which case we want local with
> &&-chaining, or it does not have to, in which case we do not want to
> have "local" that is not necessary, no?

Absolutely, my original point should have been prefixed with: I wonder
if the reason we haven't had any problems reported is because ...

And we've got lucky because the clobbering of global variables happens
not to matter in these particular cases.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] git-submodule has bash-ism?

2016-06-01 Thread John Keeping 
On Wed, Jun 01, 2016 at 03:07:59PM -0400, Jeff King wrote:
> On Wed, Jun 01, 2016 at 07:31:00PM +0100, John Keeping wrote:
> 
> > > >  reset_submodule_urls () {
> > > > -   local root
> > > > -   root=$(pwd) &&
> > > > (
> > > > +   root=$(pwd) &&
> > > > cd super-clone/submodule &&
> > > > git config remote.origin.url "$root/submodule"
> > > > ) &&
> > > > (
> > > > +   root=$(pwd) &&
> > > > cd super-clone/submodule/sub-submodule &&
> > > > git config remote.origin.url "$root/submodule"
> > [...]
> > I wonder if it's relevant that the "local root" line isn't &&-chained?
> > Is it possible that on some shells we ignore an error but everything
> > still works?
> 
> I don't think so. We're inside a function, so we wouldn't affect any
> outer &&-chaining in the function (and there isn't any in the caller
> anyway). I think it's a reasonable custom not to bother &&-chaining
> "local" lines, as they come at the top of a function and can't fail.

Can't fail if the shell supports "local", but if we're in a shell that
doesn't support it, then the lack of "&&" may allow us to just carry on.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG] git-submodule has bash-ism?

2016-06-01 Thread John Keeping 
On Wed, Jun 01, 2016 at 12:37:47PM -0400, Jeff King wrote:
> On Wed, Jun 01, 2016 at 09:19:06AM -0700, Junio C Hamano wrote:
> > diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
> > index 79bc135..5503ec0 100755
> > --- a/t/t7403-submodule-sync.sh
> > +++ b/t/t7403-submodule-sync.sh
> > @@ -62,13 +62,13 @@ test_expect_success 'change submodule' '
> >  '
> >  
> >  reset_submodule_urls () {
> > -   local root
> > -   root=$(pwd) &&
> > (
> > +   root=$(pwd) &&
> > cd super-clone/submodule &&
> > git config remote.origin.url "$root/submodule"
> > ) &&
> > (
> > +   root=$(pwd) &&
> > cd super-clone/submodule/sub-submodule &&
> > git config remote.origin.url "$root/submodule"
> 
> Hmm. Isn't $root always just going to be $TRASH_DIRECTORY here? There's
> only one caller, which appears to pass an argument which is ignored (?).
> 
> It's probably worth doing the minimal thing now and leaving further
> cleanup for a separate patch, though. Cc-ing John Keeping, the author of
> 091a6eb0feed, which added this code.

I can't shed any light on what this is trying to do; I had a look
through the mailing list and this arrived in the final version of the
series without any comment.

Looking at it now I can't see why this is a separate function (that is
called with a parameter it never uses).  I wonder if my original
approach was to call this via test_when_finished from the two tests
following this function definition, but that's pure speculation now.

Junio's change is obviously correct as a minimal fix.

I wonder if it's relevant that the "local root" line isn't &&-chained?
Is it possible that on some shells we ignore an error but everything
still works?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Force-with-lease and new branches

2016-05-07 Thread John Keeping 
I've noticed a slightly annoying behaviour of git-push's
--force-with-lease option around branch creation.

I'd like to be able to do:

git push --force-with-lease origin refs/heads/jk/*

to push out a load of topic branches safely in case I've switched client
machines and forgotten to pull, but for newly-created branches this
fails with "stale-info", which seems to be intentional via the
expect_old_no_trackback field in struct ref.

However, if I use the explicit --force-with-lease syntax with the null
hash then the push does succeed.  I've added a couple of tests to t5533
which demonstrate this in a patch below - the first one fails but the
second passes.

It looks like this has been the case since the first version of what
would become --force-with-lease [1] and I can't find any discussion
around this particular behaviour in the three versions of that patch set
I found on Gmane [2], [3], [4].

So my questions are: what will break if we decide to treat "no remote
tracking branch" as "new branch" and is that a reasonable thing to do?


[1] http://article.gmane.org/gmane.comp.version-control.git/229992
[2] http://article.gmane.org/gmane.comp.version-control.git/229430
[3] http://article.gmane.org/gmane.comp.version-control.git/230142
[4] http://article.gmane.org/gmane.comp.version-control.git/231021


-- >8 --
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index c732012..ad9e06f 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -191,4 +191,28 @@ test_expect_success 'cover everything with default 
force-with-lease (allowed)' '
test_cmp expect actual
 '
 
+test_expect_success 'new branch covered by force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push --force-with-lease=branch origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'new branch with explicit force-with-lease' '
+   setup_srcdst_basic &&
+   (
+   cd dst &&
+   git branch branch master &&
+   git push 
--force-with-lease=branch: origin branch
+   ) &&
+   git ls-remote dst refs/heads/branch >expect &&
+   git ls-remote src refs/heads/branch >actual &&
+   test_cmp expect actual
+'
+
 test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: history damage in linux.git

2016-04-21 Thread John Keeping 
On Thu, Apr 21, 2016 at 01:30:04PM +0200, Olaf Hering wrote:
> To track the changes in hyperv related files I created some scripts
> years ago to automate the process of finding relevant commits in
> linux.git. Part of that process is to record the tag when a commit
> appeared in mainline. This worked fine, until very recently.
> 
> Suddenly years-old commits are declared as having-just-arrived in
> linux.git. Look at this example:
> 
>   $ git log --oneline -- drivers/input/serio/hyperv-keyboard.c
>   2048157 Drivers: hv: vmbus: fix the building warning with hyperv-keyboard
>   62238f3 Input: hyperv-keyboard - register as a wakeup source
>   c3c4d99 Input: hyperv-keyboard - pass through 0xE1 prefix
>   aed06b9 Input: add a driver to support Hyper-V synthetic keyboard
>   $ git describe --contains aed06b9
>   v4.6-rc1~9^2~792
>   $ git show aed06b9 | head
>   commit aed06b9cfcabf8644ac5f6f108c0b3d01522f88b
>   Author: K. Y. Srinivasan 
>   Date:   Wed Sep 18 12:50:42 2013 -0700
> 
> Obviously that and other commits are in the tree since a very long time.
> 
> How can I find out whats going on? Is my git(1) 2.8.1 broken, or did
> Linus just pull some junk tree (and does he continue to do so)?

I suspect it indicates that an old tree was pulled in such that the path
to v4.6-rc1 is shorter than to the older version.  The commit is clearly
in v3.13-rc1:

$ git tag --contains aed06b9cfcabf8644ac5f6f108c0b3d01522f88b
v3.13
v3.13-rc1
v3.13-rc2
[snip]

The behaviour of describe is a bit clearer if you limit it to v3.*:

$ git describe --match='v3.*' --contains 
aed06b9cfcabf8644ac5f6f108c0b3d01522f88b
v3.13-rc7~9^2~14^2~42

$ git describe --match='v3.13-rc1' --contains 
aed06b9cfcabf8644ac5f6f108c0b3d01522f88b
v3.13-rc1~65^2^2~42

It seems that the path to v4.6-rc1 is "more direct" than to either of
these commits: there is only one second-parent merge transition.

>From a quick look, I think the problem is in commit c155c7492c9a ("Merge
branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input")
which merges a branch that has repeatedly had master merged back into it
but does not build on any recent releases.  The most recent tag on the
first-parent history of that branch is v3.0-rc4.

I think it is as simple as git-describe (or git-name-rev which is used
in the --contains case) preferring a less branchy path, which has been
introduced in v4.6 with the merge commit above.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

2016-04-12 Thread John Keeping 
On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Fo wrote:
> On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote:
> > I think the case in these two is correct as-is.  The "Y" is capitalised
> > because it is the default and will take effect if the user just presses
> > ENTER.
> 
> Thanks John, I'm aware of that. That's why the patch doesn't change
> the case. Maybe I should have mention that explicitly in the commit
> message.

Sorry, I completely missed that.  Your patch does in fact look good, so:

Reviewed-by: John Keeping <j...@keeping.me.uk>

I think I was taken in by the commit message saying 'i.e. "Question
[y/n]? "' and didn't examine the patch carefully enough.  It might be
better just to drop the example since it's obvious what the patch does.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

2016-04-12 Thread John Keeping 
On Tue, Apr 12, 2016 at 02:59:42PM +0200, Nikola Fo wrote:
> Every yes/no question in difftool/mergetool scripts has slightly
> different form, and none of them is consistent with the form git
> itself uses.
> 
> Make the form of all the questions consistent with the form used
> by git, i.e. "Question [y/n]? ".
> 
> Signed-off-by: Nikola Forró 
> ---
>  git-difftool--helper.sh | 4 ++--
>  git-mergetool--lib.sh   | 2 +-
>  git-mergetool.sh| 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 2b11b1d..84d6cc0 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -44,10 +44,10 @@ launch_merge_tool () {
>   "$GIT_DIFF_PATH_TOTAL" "$MERGED"
>   if use_ext_cmd
>   then
> - printf "Launch '%s' [Y/n]: " \
> + printf "Launch '%s' [Y/n]? " \
>   "$GIT_DIFFTOOL_EXTCMD"
>   else
> - printf "Launch '%s' [Y/n]: " "$merge_tool"
> + printf "Launch '%s' [Y/n]? " "$merge_tool"

I think the case in these two is correct as-is.  The "Y" is capitalised
because it is the default and will take effect if the user just presses
ENTER.

>From a quick look, the two cases below do not have this behaviour (the
user must enter either "y" or "n"), so it is correct that they are not
capitalized.

The change from ":" to "?" and normalization of "?" placement below
seems reasonable.

>   fi
>   read ans || return
>   if test "$ans" = n
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 54ac8e4..92adcc0 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -100,7 +100,7 @@ check_unchanged () {
>   while true
>   do
>   echo "$MERGED seems unchanged."
> - printf "Was the merge successful? [y/n] "
> + printf "Was the merge successful [y/n]? "
>   read answer || return 1
>   case "$answer" in
>   y*|Y*) return 0 ;;
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index 9f77e3a..2e0635a 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -396,7 +396,7 @@ done
>  prompt_after_failed_merge () {
>   while true
>   do
> - printf "Continue merging other unresolved paths (y/n) ? "
> + printf "Continue merging other unresolved paths [y/n]? "
>   read ans || return 1
>   case "$ans" in
>   [yY]*)
> -- 
> 2.4.11
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git config not working correctly with included configurations

2016-04-12 Thread John Keeping 
On Tue, Apr 12, 2016 at 08:25:39AM -0300, Rudinei Goi Roecker wrote:
> I'm having a problem with included configurations in ~/.gitconfig, when 
> using this command:
> 
> git config --global user.email


> It doesn't return anything, in commits it works as intended. The 
> configuration looks like this:

I think you need:

git config --global --includes user.email

or simply:

git config user.email

See the documentation of the --includes and --no-includes options in
git-config(1).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 11:00:03PM +0100, John Keeping wrote:
> On Tue, Mar 29, 2016 at 05:21:43PM -0400, Jeff King wrote:
> > On Tue, Mar 29, 2016 at 09:52:08PM +0100, John Keeping wrote:
> > 
> > > > Yeah, I think this is a bug. Presumably what is happening is that we are
> > > > too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> > > > we ask "are we in a work tree", the answer has become yes. But the
> > > > caller really wants to know "am _I_ inside the work tree".
> > > 
> > > I don't think that's what's happening.  Try:
> > > 
> > >   $ cd .git/
> > >   $ GIT_WORK_TREE=.. git rev-parse --is-inside-work-tree
> > >   true
> > > 
> > > so I think it's that we refuse to assume that the directory above a Git
> > > directory is a working tree (something similar happens when the
> > > "core.worktree" config variable is set).  I'm not convinced that's
> > > unreasonable.
> > 
> > Yeah, you're right, but I'm not sure how your example shows that, (isn't
> > it basically the same as Elliott's original, except using a relative
> > path?). A more compelling counter-example to my hypothesis is:
> > 
> >   $ cd .git
> >   $ GIT_WORK_TREE=/tmp git rev-parse --is-inside-work-tree
> >   false
> > 
> > So it is not that we chdir too early, but just that we blindly check "is
> > $(pwd) inside $GIT_WORK_TREE". And it does not create a problem for the
> > normal discovered-path cases, because either:
> > 
> >   - we discovered .git by walking up the directory tree, which means we
> > must be in a work-tree
> > 
> >   - we discovered that we are inside a .git directory, and therefore
> > take it to be bare (and thus there is no work tree, and we cannot be
> > inside it). This is what happens in Elliott's original example that
> > behaves differently than the $GIT_WORK_TREE case.
> > 
> > I'd be tempted to say that "inside the work tree" is further clarified
> > to "not inside the $GIT_DIR".
> 
> Yes, I think that's reasonable.  But...
> 
> > > However, the case above also gives:
> > > 
> > >   $ GIT_WORK_TREE=.. git rev-parse --is-inside-git-dir
> > >   false
> > >   $ test $(pwd) = $(GIT_WORK_TREE=.. git rev-parse --git-dir); echo $?
> > >   0
> > > 
> > > so even though $PWD *is* the Git directory, we're not in the Git
> > > directory!  Setting GIT_DIR=$(pwd) makes no different to that.
> > 
> > We seem to get that wrong. I'm also not sure if it would make sense if
> > you explicitly set the two to be equal, like:
> > 
> >   # checking in your own refs?
> >   GIT_WORK_TREE=$(pwd) GIT_DIR=$(pwd) git add refs packed-refs
> > 
> > So the current behavior may just be weird-but-true.
> 
> This case definitely feels wrong:
> 
>   $ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git rev-parse 
> --is-inside-git-dir
>   false
> 
> Shouldn't that be the same as if GIT_WORK_TREE and GIT_DIR aren't set?
> (It's also potentially surprising since "git rev-parse --git-dir" does
> give the right answer in this case.)
> 
> If GIT_WORK_TREE points somewhere unrelated then it is correct:
> 
>   $ GIT_WORK_TREE=/tmp GIT_DIR=$(pwd) git rev-parse --is-inside-git-dir
>   true

It seems that this is a result of changing the working directory to the
root of the working tree if we're inside it.  is_inside_dir() doesn't
take account of startup_info->prefix and changing to:

real_path(startup_info->prefix)

instead of xgetcwd() means that these tests are less surprising.

But I haven't run the test suite or thought about what else this could
break.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 05:21:43PM -0400, Jeff King wrote:
> On Tue, Mar 29, 2016 at 09:52:08PM +0100, John Keeping wrote:
> 
> > > Yeah, I think this is a bug. Presumably what is happening is that we are
> > > too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> > > we ask "are we in a work tree", the answer has become yes. But the
> > > caller really wants to know "am _I_ inside the work tree".
> > 
> > I don't think that's what's happening.  Try:
> > 
> > $ cd .git/
> > $ GIT_WORK_TREE=.. git rev-parse --is-inside-work-tree
> > true
> > 
> > so I think it's that we refuse to assume that the directory above a Git
> > directory is a working tree (something similar happens when the
> > "core.worktree" config variable is set).  I'm not convinced that's
> > unreasonable.
> 
> Yeah, you're right, but I'm not sure how your example shows that, (isn't
> it basically the same as Elliott's original, except using a relative
> path?). A more compelling counter-example to my hypothesis is:
> 
>   $ cd .git
>   $ GIT_WORK_TREE=/tmp git rev-parse --is-inside-work-tree
>   false
> 
> So it is not that we chdir too early, but just that we blindly check "is
> $(pwd) inside $GIT_WORK_TREE". And it does not create a problem for the
> normal discovered-path cases, because either:
> 
>   - we discovered .git by walking up the directory tree, which means we
> must be in a work-tree
> 
>   - we discovered that we are inside a .git directory, and therefore
> take it to be bare (and thus there is no work tree, and we cannot be
> inside it). This is what happens in Elliott's original example that
> behaves differently than the $GIT_WORK_TREE case.
> 
> I'd be tempted to say that "inside the work tree" is further clarified
> to "not inside the $GIT_DIR".

Yes, I think that's reasonable.  But...

> > However, the case above also gives:
> > 
> > $ GIT_WORK_TREE=.. git rev-parse --is-inside-git-dir
> > false
> > $ test $(pwd) = $(GIT_WORK_TREE=.. git rev-parse --git-dir); echo $?
> > 0
> > 
> > so even though $PWD *is* the Git directory, we're not in the Git
> > directory!  Setting GIT_DIR=$(pwd) makes no different to that.
> 
> We seem to get that wrong. I'm also not sure if it would make sense if
> you explicitly set the two to be equal, like:
> 
>   # checking in your own refs?
>   GIT_WORK_TREE=$(pwd) GIT_DIR=$(pwd) git add refs packed-refs
> 
> So the current behavior may just be weird-but-true.

This case definitely feels wrong:

$ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git rev-parse 
--is-inside-git-dir
false

Shouldn't that be the same as if GIT_WORK_TREE and GIT_DIR aren't set?
(It's also potentially surprising since "git rev-parse --git-dir" does
give the right answer in this case.)

If GIT_WORK_TREE points somewhere unrelated then it is correct:

$ GIT_WORK_TREE=/tmp GIT_DIR=$(pwd) git rev-parse --is-inside-git-dir
true
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 04:34:25PM -0400, Jeff King wrote:
> On Tue, Mar 29, 2016 at 06:42:44AM -0500, Elliott Cable wrote:
> 
> > So, I find this behaviour a little strange; I can't determine if it's
> > a subtle bug, or intentionally undefined/‘fuzzy’ behaviour:
> > 
> > $ cd a-repo/.git/
> > $ pwd
> > /path/to/a-repo/.git
> > $ git rev-parse --is-inside-work-tree
> > false
> > $ export GIT_WORK_TREE=/path/to/a-repo
> > $ git rev-parse --is-inside-work-tree
> > true
> > 
> > i.e. when within the repository (the `.git` directory), and when that
> > directory is a sub-directory of the working-tree, `rev-parse
> > --is-inside-work-tree` reports *false* (reasonable enough, I suppose);
> > but then if `$GIT_WORK_TREE` is set to precisely the directory that
> > git was *already* assuming was the working-directory, then the same
> > command, in the same location, reports *true*.
> > 
> > This should probably be made consistent: either `rev-parse
> > --is-inside-work-tree` should report “true”, even inside the `.git`
> > dir, as long as that directory is a sub-directory of the working-tree
> > … or repository-directories / `$GIT_DIR` / `.git` directories should
> > be excluded from truthy responses to `rev-parse
> > --is-inside-work-tree`.
> 
> Yeah, I think this is a bug. Presumably what is happening is that we are
> too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> we ask "are we in a work tree", the answer has become yes. But the
> caller really wants to know "am _I_ inside the work tree".

I don't think that's what's happening.  Try:

$ cd .git/
$ GIT_WORK_TREE=.. git rev-parse --is-inside-work-tree
true

so I think it's that we refuse to assume that the directory above a Git
directory is a working tree (something similar happens when the
"core.worktree" config variable is set).  I'm not convinced that's
unreasonable.

However, the case above also gives:

$ GIT_WORK_TREE=.. git rev-parse --is-inside-git-dir
false
$ test $(pwd) = $(GIT_WORK_TREE=.. git rev-parse --git-dir); echo $?
0

so even though $PWD *is* the Git directory, we're not in the Git
directory!  Setting GIT_DIR=$(pwd) makes no different to that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] worktree: add: introduce --checkout option

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 02:04:38PM -0400, Eric Sunshine wrote:
> On Tue, Mar 29, 2016 at 6:54 AM, John Keeping <j...@keeping.me.uk> wrote:
> > On Tue, Mar 29, 2016 at 10:11:01AM +, Ray Zhang wrote:
> >>   With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
> >>   in linkgit:git-checkout[1].
> >>
> >> +--[no-]checkout::
> >
> > This should be:
> >
> > --checkout::
> > --no-checkout::
> >
> > (see for example --progress in Documentation/merge-options.txt).
> 
> [1] suggested either form without stating a preference since existing
> Git documentation uses a mixture of the two. See, for instance,
> git-format-patch.txt. However, I see now that --[no-]-option is the
> minority.
> 
> [1]: http://article.gmane.org/gmane.comp.version-control.git/289840

I tend to skim the mailing list so I didn't register that at the time.

Having gone looking, I can't find a reference but I for some reason I
was convinced the separate version was preferred in the option
descriptions.  Note that AsciiDoc does handle this specially, at least
when outputting troff (HTML seems to show both on separate lines).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 06:53:35AM -0500, Elliott Cable wrote:
> On Tue, Mar 29, 2016 at 6:42 AM, Elliott Cable  wrote:
> > So, I find this behaviour a little strange; I can't determine if it's
> > a subtle bug, or intentionally undefined/‘fuzzy’ behaviour ...
> 
> Oh lord, it gets worse ...
> 
> $ cd a-repo
> $ git rev-parse --is-inside-work-tree; git rev-parse --is-inside-git-dir
> true
> false
> $ cd .git
> $ git rev-parse --is-inside-work-tree; git rev-parse --is-inside-git-dir
> false
> true

I believe these are working correctly, the .git directory is not part of
the working tree.

> $ export GIT_WORK_TREE="$(git rev-parse --show-toplevel)"   # !!!

Did you check the value of GIT_WORK_TREE here?  When I try it's the
empty string.

If I set the core.worktree config variable to ".." then rev-parse does
find the working tree correctly.  I recall some previous discussion
about this but I can't find it in the list archives from a quick search.

> $ git rev-parse --is-inside-work-tree; git rev-parse --is-inside-git-dir
> true
> false
> $ # !!?!?
> 
> So, basically, if `$GIT_WORK_TREE` is set at all, it appears that the
> `rev-parse --is-inside...` flags don't function reliably at all.

If you set GIT_WORK_TREE you're telling Git to override all of the
normal detection logic.  What version of Git are you using?  When I try
this it says:

fatal: The empty string is not a valid path

If I set GIT_WORK_TREE to the correct value for this repository then it
behaves the same as with the auto-detection logic.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5] worktree: add: introduce --checkout option

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 10:11:01AM +, Ray Zhang wrote:
> By adding this option which defaults to true, we can use the
> corresponding --no-checkout to make some customizations before
> the checkout, like sparse checkout, etc.
> 
> Helped-by: Eric Sunshine 
> Helped-by: Junio C Hamano 
> Reviewed-by: Eric Sunshine 
> Signed-off-by: Ray Zhang 
> ---
> Changes since last version of this patch[v4]:
>   t/t2025-worktree-add.sh: use test -e to test file existence.
>   builtin/worktree.c: refactor the code a little bit.
> 
> [v4]: http://article.gmane.org/gmane.comp.version-control.git/290030
> [v3]: http://article.gmane.org/gmane.comp.version-control.git/289877
> [v2]: http://article.gmane.org/gmane.comp.version-control.git/289713
> [v1]: http://article.gmane.org/gmane.comp.version-control.git/289659
> ---
>  Documentation/git-worktree.txt |  8 +++-
>  builtin/worktree.c | 29 ++---
>  t/t2025-worktree-add.sh| 12 
>  3 files changed, 37 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> index 62c76c1..c622345 100644
> --- a/Documentation/git-worktree.txt
> +++ b/Documentation/git-worktree.txt
> @@ -9,7 +9,7 @@ git-worktree - Manage multiple working trees
>  SYNOPSIS
>  
>  [verse]
> -'git worktree add' [-f] [--detach] [-b ]  []
> +'git worktree add' [-f] [--detach] [--checkout] [-b ]  
> []
>  'git worktree prune' [-n] [-v] [--expire ]
>  'git worktree list' [--porcelain]
>  
> @@ -87,6 +87,12 @@ OPTIONS
>   With `add`, detach HEAD in the new working tree. See "DETACHED HEAD"
>   in linkgit:git-checkout[1].
>  
> +--[no-]checkout::

This should be:

--checkout::
--no-checkout::

(see for example --progress in Documentation/merge-options.txt).

> + By default, `add` checks out ``, however, `--no-checkout` can
> + be used to suppress checkout in order to make customizations,
> + such as configuring sparse-checkout. See "Sparse checkout"
> + in linkgit:git-read-tree[1].
> +
>  -n::
>  --dry-run::
>   With `prune`, do not remove anything; just report what it would
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 38b5609..d8e3795 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -21,6 +21,7 @@ static const char * const worktree_usage[] = {
>  struct add_opts {
>   int force;
>   int detach;
> + int checkout;
>   const char *new_branch;
>   int force_new_branch;
>  };
> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char 
> *refname,
>   if (ret)
>   goto done;
>  
> - cp.argv = NULL;
> - argv_array_clear();
> - argv_array_pushl(, "reset", "--hard", NULL);
> - cp.env = child_env.argv;
> - ret = run_command();
> - if (!ret) {
> - is_junk = 0;
> - free(junk_work_tree);
> - free(junk_git_dir);
> - junk_work_tree = NULL;
> - junk_git_dir = NULL;
> + if (opts->checkout) {
> + cp.argv = NULL;
> + argv_array_clear();
> + argv_array_pushl(, "reset", "--hard", NULL);
> + cp.env = child_env.argv;
> + ret = run_command();
> + if (ret)
> + goto done;
>   }
> +
> + is_junk = 0;
> + free(junk_work_tree);
> + free(junk_git_dir);
> + junk_work_tree = NULL;
> + junk_git_dir = NULL;
> +
>  done:
>   strbuf_reset();
>   strbuf_addf(, "%s/locked", sb_repo.buf);
> @@ -320,10 +325,12 @@ static int add(int ac, const char **av, const char 
> *prefix)
>   OPT_STRING('B', NULL, _branch_force, N_("branch"),
>  N_("create or reset a branch")),
>   OPT_BOOL(0, "detach", , N_("detach HEAD at named 
> commit")),
> + OPT_BOOL(0, "checkout", , N_("populate the new 
> working tree")),
>   OPT_END()
>   };
>  
>   memset(, 0, sizeof(opts));
> + opts.checkout = 1;
>   ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
>   if (!!opts.detach + !!opts.new_branch + !!new_branch_force > 1)
>   die(_("-b, -B, and --detach are mutually exclusive"));
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index cbfa41e..3acb992 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -213,4 +213,16 @@ test_expect_success 'local clone from linked checkout' '
>   ( cd here-clone && git fsck )
>  '
>  
> +test_expect_success '"add" worktree with --no-checkout' '
> + git worktree add --no-checkout -b swamp swamp &&
> + ! test -e swamp/init.t &&
> + git -C swamp reset --hard &&
> + test_cmp init.t swamp/init.t
> +'
> +
> +test_expect_success '"add" worktree with --checkout' '
> + git worktree add --checkout -b swmap2 swamp2 &&
> + test_cmp

Re: `git rev-parse --git-dir` relative to current working directory?

2016-03-29 Thread John Keeping 
On Tue, Mar 29, 2016 at 05:32:31AM -0500, Elliott Cable wrote:
> So, `git help rev-parse` [mentions the following][rev-parse], as of
> 2.8.0:
> 
> --git-dir
>Show $GIT_DIR if defined. Otherwise show the path to the .git
>directory. The path shown, when relative, is relative to the
>current working directory.
> 
> However, when inside a symlinked repository, this doesn't function as
> advertised:
> 
> $ ln -s a-symlink a-git-repo
> $ cd a-symlink/.git/hooks
> $ git rev-parse --git-dir
> /Users/ec/Documents/a-git-repo/.git
> 
> From my reading of that snippet of documentation (“The path shown ... is
> relative to the CWD”), I'd expect to receive `..`, not
> `/absolute/path/to/a-git-repo/.git`.
> 
> Is the documentation incorrect, or is this a bug? (I'm hoping the
> latter: I'm trying to write git-scripting that is sensitive to symlinks,
> i.e. retains the user's CWD without unintentionally resolving symlinks
> in their path during operation; and it'd be ideal if this were handled
> as documented, saving me manual effort checking symlinks.)

The documentation seems correct to me, it just requires careful parsing;
I read it as:

if the path printed it relative
then it is relative to the current working directory

but it makes not claims about when a relative path will be printed (or
even if one ever will be, although in my testing the path is relative
only if $CWD can be stripped from the beginning of the path; in other
words only when no component of the relative path would be "../").
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] Code reorgnization

2016-03-19 Thread John Keeping 
On Thu, Mar 17, 2016 at 12:10:44PM -0700, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> > For now I would just go with 3 directories:
> >
> > non-git/ (or util, helpers, or anything that could be ripped out and be 
> > useful
> > e.g. strbufs, argv-array run-command, lockfile
> > git/ (maybe called lib? All stuff that is pure Git and is used for libgit
> >
> > builtin/ (as we have it today + all that stuff that doesn't go into
> > git/ very well?)
> 
> It is unclear where you want to have standalone programs in the
> above.  I'd say lib/ and src/ for the first two, where lib/ is for
> things that could be lifted without any Git dependencies and src/
> for everything else.
> 
> Aren't there some folks who link directly with our codebase (I am
> thinking about cgit, but hjemli.net/git/cgit does not seem to be
> responding anymore)?

CGit lives at https://git.zx2c4.com/cgit/ these days.

The organisation of the git code shouldn't make a difference since CGit
just links with libgit.a, even if it does CGit pulls in git.git as a
submodule so it can just fix any problems in the same commit that
updates the submodule reference.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Feature request: Configurable prefixes for git commit --fixup and --squash

2016-03-03 Thread John Keeping 
[please don't top post on this list]

On Thu, Mar 03, 2016 at 03:33:34PM +0100, Martine Lenders wrote:
> yes, it can be anywhere in the commit message and I already thought
> about using a hook for generating the commit message too, but the
> problem is then, that `git rebase` won't pair up the commit for
> squashing/fixing up with the original commit.

Huh?  With the hook below you just run `git commit --fixup=...` as
normal and it appends a "[ci skip]" line to the bottom of the commit
message.

> Am 03.03.2016 14:21 schrieb "John Keeping" <j...@keeping.me.uk>:
> >
> > On Thu, Mar 03, 2016 at 01:47:00PM +0100, Martine Lenders wrote:
> > > I'm not sure if this was already requested somewhere (a quick - but
> > > admittedly not thorough - search did not reveal anything in that
> > > direction), but I really miss an option to configure the prefixes 
> > > generated
> > > by `git commit (--fixup | --squash) ` and picked up by `git rebase
> > > -i --autosquash`.
> > >
> > > My reasoning is that in our project we use GitHub + Travis to test-build
> > > our pull requests, but we don't want to spam the CI server with builds 
> > > that
> > > are just fixups to previous changes (which are uploaded so reviewers can
> > > track the changes to the original PR). Now, Travis has the option to not
> > > build a commit if there is the string `[ci skip]` in the commit message
> > > (sadly also not configurable) so it would be really great for my workflow
> > > if I could just add this string to the message generated by `--fixup`.
> >
> > I am against the feature as you describe it, because it has the
> > potential to break `git rebase --autosquash` with shared fixups if two
> > people are using a different prefix.
> >
> > However, it sounds like Travis will recognize "[ci skip]" anywhere in
> > the commit message.  Would a feature to allow autogenerated content in
> > fixup/squash commit message bodies work?
> >
> > In fact, this can already be achieved with a prepare-commit-msg hook
> > like this (untested, but shows the principle):
> >
> > -- >8 --
> > #!/bin/sh
> > case "$(head -n 1 "$1")" in
> > "fixup! "*|"squash! "*)
> > cat >>"$1" <<-\EOF
> >
> > [ci skip]
> > EOF
> > esac
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Feature request: Configurable prefixes for git commit --fixup and --squash

2016-03-03 Thread John Keeping 
On Thu, Mar 03, 2016 at 01:47:00PM +0100, Martine Lenders wrote:
> I'm not sure if this was already requested somewhere (a quick - but
> admittedly not thorough - search did not reveal anything in that
> direction), but I really miss an option to configure the prefixes generated
> by `git commit (--fixup | --squash) ` and picked up by `git rebase
> -i --autosquash`.
> 
> My reasoning is that in our project we use GitHub + Travis to test-build
> our pull requests, but we don't want to spam the CI server with builds that
> are just fixups to previous changes (which are uploaded so reviewers can
> track the changes to the original PR). Now, Travis has the option to not
> build a commit if there is the string `[ci skip]` in the commit message
> (sadly also not configurable) so it would be really great for my workflow
> if I could just add this string to the message generated by `--fixup`.

I am against the feature as you describe it, because it has the
potential to break `git rebase --autosquash` with shared fixups if two
people are using a different prefix.

However, it sounds like Travis will recognize "[ci skip]" anywhere in
the commit message.  Would a feature to allow autogenerated content in
fixup/squash commit message bodies work?

In fact, this can already be achieved with a prepare-commit-msg hook
like this (untested, but shows the principle):

-- >8 --
#!/bin/sh
case "$(head -n 1 "$1")" in
"fixup! "*|"squash! "*)
cat >>"$1" <<-\EOF

[ci skip]
EOF
esac
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Trouble Cloning Git remote repository

2016-03-01 Thread 'John Keeping' 
On Tue, Mar 01, 2016 at 10:04:49AM -0500, Fred's Personal wrote:
> Thanks for the insight, it's been a while since I debugged a Windows call
> stack. Can you give me commands to view what gets passed to
> CreateProcessW().

Sorry, my Windows knowledge is several years old.  Maybe procmon[1] will
show them?

[1] https://technet.microsoft.com/en-us/sysinternals/processmonitor.aspx

> -Original Message-----
> From: John Keeping [mailto:j...@keeping.me.uk] 
> Sent: Monday, February 29, 2016 4:35 AM
> To: Duy Nguyen
> Cc: Fred's Personal; Git Mailing List
> Subject: Re: Trouble Cloning Git remote repository
> 
> On Mon, Feb 29, 2016 at 08:20:46AM +0700, Duy Nguyen wrote:
> > On Mon, Feb 29, 2016 at 12:48 AM, Fred's Personal 
> > <freddie...@optonline.net> wrote:
> > > Duy,
> > >
> > > Thanks for advice, here is the result of executing the lines you
> suggested:
> > >
> > > user1@Host1 MINGW64 ~/gitrepository (master) $ export GIT_TRACE=1
> > >
> > > user1@Host1 MINGW64 ~/gitrepository (master) $ git clone -v 
> > > ssh://user1@Host2/srv/centralrepo
> > > 12:33:47.928365 git.c:348   trace: built-in: git 'clone'
> '-v' 'ssh://user1@Host2/srv/centralrepo'
> > > Cloning into 'centralrepo'...
> > > 12:33:48.022110 run-command.c:343   trace: run_command: 'C:\Program
> Files (x86)\PuTTY\plink.exe' 'user1@Host2' 'git-upload-pack
> '\''/srv/centralrepo'\'''
> > 
> > This command looks good to me. So I have no clue what goes wrong :-) 
> > Maybe you can execute this command directly, with more plink debugging 
> > options or something. You don't have to run git-upload-pack. Just 
> > spawn a shell. Another option is try something other than plink (does 
> > git-for-windows come with ssh.exe?)
> 
> On Windows it's probably going through mingw_spawnve_fd() which reassembles
> a quoted command line from the individual arguments.  It would be
> interesting to see what gets passed to CreateProcessW().
> 
> > > ##>>>Lines from $HOME/.bashrc (See below, removed here for clarity)
> > >
> > > + user1@Host2 git-upload-pack /srv/centralrepo
> > > bash: user1@Host2: command not found
> > > fatal: Could not read from remote repository.
> > >
> > > Please make sure you have the correct access rights and the 
> > > repository exists.
> > >
> > >
> > > Regards,
> > > Fred
> > >
> > > freddie...@optonline.net
> > >
> > >
> > > -Original Message-
> > > From: Duy Nguyen [mailto:pclo...@gmail.com]
> > > Sent: Saturday, February 27, 2016 4:36 AM
> > > To: Fred's Personal
> > > Cc: Git Mailing List
> > > Subject: Re: Trouble Cloning Git remote repository
> > >
> > > On Sat, Feb 27, 2016 at 6:03 AM, Fred's Personal
> <freddie...@optonline.net> wrote:
> > >> $ git clone -v ssh://user1@Host2/srv/centralrepo Cloning into 
> > >> 'centralrepo'...
> > >>>>>Lines from $HOME/.bashrc
> > >>   + export
> > >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> > >> usr
> > >> /games
> > >> :/usr/local/games
> > >>   +
> > >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/
> > >> usr
> > >> /games
> > >> :/usr/local/games
> > >>   + PROMPT_COMMAND=
> > >>   + CDPATH=
> > >>   + '[' '' = yes ']'
> > >>   + PS1='${debian_chroot:+($debian_chroot)}\u:\W\$ '
> > >>   + export GIT_TRACE_PACKET=1
> > >>   + GIT_TRACE_PACKET=1
> > >>   + export GIT_TRACE=1
> > >>   + GIT_TRACE=1
> > >>>>>End of Lines from $HOME/.bashrc
> > >> ## WHERE DOES The following line COME FROMWhat Script spits out 
> > >> this line
> > >>   + user1@Host2 git-upload-pack /srv/centralrepo
> > >
> > > Try set GIT_TRACE=1 at the clone line, I have a feeling that this line
> should be "ssh user@Host2..." but "ssh" is missing.
> > >
> > > $ export GIT_TRACE=1
> > > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > > --
> > > Duy
> > >
> > >
> > > -
> > > No virus found in this message.
> > > Checked by AVG - www.avg.com
> > > Version: 2016.0.7442 / Virus Database: 4537/11702 - Release Date: 
> > > 02/26/16
> > >
> > >
> > 
> > 
> > 
> > --
> > Duy
> > --
> > To unsubscribe from this list: send the line "unsubscribe git" in the 
> > body of a message to majord...@vger.kernel.org More majordomo info at  
> > http://vger.kernel.org/majordomo-info.html
> 
> 
> -
> No virus found in this message.
> Checked by AVG - www.avg.com
> Version: 2016.0.7442 / Virus Database: 4537/11716 - Release Date: 02/28/16
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Trouble Cloning Git remote repository

2016-02-29 Thread John Keeping 
On Mon, Feb 29, 2016 at 08:20:46AM +0700, Duy Nguyen wrote:
> On Mon, Feb 29, 2016 at 12:48 AM, Fred's Personal
>  wrote:
> > Duy,
> >
> > Thanks for advice, here is the result of executing the lines you suggested:
> >
> > user1@Host1 MINGW64 ~/gitrepository (master)
> > $ export GIT_TRACE=1
> >
> > user1@Host1 MINGW64 ~/gitrepository (master)
> > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > 12:33:47.928365 git.c:348   trace: built-in: git 'clone' '-v' 
> > 'ssh://user1@Host2/srv/centralrepo'
> > Cloning into 'centralrepo'...
> > 12:33:48.022110 run-command.c:343   trace: run_command: 'C:\Program 
> > Files (x86)\PuTTY\plink.exe' 'user1@Host2' 'git-upload-pack 
> > '\''/srv/centralrepo'\'''
> 
> This command looks good to me. So I have no clue what goes wrong :-)
> Maybe you can execute this command directly, with more plink debugging
> options or something. You don't have to run git-upload-pack. Just
> spawn a shell. Another option is try something other than plink (does
> git-for-windows come with ssh.exe?)

On Windows it's probably going through mingw_spawnve_fd() which
reassembles a quoted command line from the individual arguments.  It
would be interesting to see what gets passed to CreateProcessW().

> > ##>>>Lines from $HOME/.bashrc (See below, removed here for clarity)
> >
> > + user1@Host2 git-upload-pack /srv/centralrepo
> > bash: user1@Host2: command not found
> > fatal: Could not read from remote repository.
> >
> > Please make sure you have the correct access rights
> > and the repository exists.
> >
> >
> > Regards,
> > Fred
> >
> > freddie...@optonline.net
> >
> >
> > -Original Message-
> > From: Duy Nguyen [mailto:pclo...@gmail.com]
> > Sent: Saturday, February 27, 2016 4:36 AM
> > To: Fred's Personal
> > Cc: Git Mailing List
> > Subject: Re: Trouble Cloning Git remote repository
> >
> > On Sat, Feb 27, 2016 at 6:03 AM, Fred's Personal  
> > wrote:
> >> $ git clone -v ssh://user1@Host2/srv/centralrepo Cloning into
> >> 'centralrepo'...
> >Lines from $HOME/.bashrc
> >>   + export
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr
> >> /games
> >> :/usr/local/games
> >>   +
> >> PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr
> >> /games
> >> :/usr/local/games
> >>   + PROMPT_COMMAND=
> >>   + CDPATH=
> >>   + '[' '' = yes ']'
> >>   + PS1='${debian_chroot:+($debian_chroot)}\u:\W\$ '
> >>   + export GIT_TRACE_PACKET=1
> >>   + GIT_TRACE_PACKET=1
> >>   + export GIT_TRACE=1
> >>   + GIT_TRACE=1
> >End of Lines from $HOME/.bashrc
> >> ## WHERE DOES The following line COME FROMWhat Script spits out
> >> this line
> >>   + user1@Host2 git-upload-pack /srv/centralrepo
> >
> > Try set GIT_TRACE=1 at the clone line, I have a feeling that this line 
> > should be "ssh user@Host2..." but "ssh" is missing.
> >
> > $ export GIT_TRACE=1
> > $ git clone -v ssh://user1@Host2/srv/centralrepo
> > --
> > Duy
> >
> >
> > -
> > No virus found in this message.
> > Checked by AVG - www.avg.com
> > Version: 2016.0.7442 / Virus Database: 4537/11702 - Release Date: 02/26/16
> >
> >
> 
> 
> 
> -- 
> Duy
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] Documentation/git-config: fix --get-all description

2016-02-28 Thread John Keeping 
--get does not fail if a key is multi-valued, it returns the last value
as described in its documentation.  Clarify the description of --get-all
to avoid implying that --get does fail in this case.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 Documentation/git-config.txt | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e9c755f..6fc08e3 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -86,8 +86,7 @@ OPTIONS
found and the last value if multiple key values were found.
 
 --get-all::
-   Like get, but does not fail if the number of values for the key
-   is not exactly one.
+   Like get, but returns all values for a multi-valued key.
 
 --get-regexp::
Like --get-all, but interprets the name as a regular expression and
-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] Documentation/git-config: use bulleted list for exit codes

2016-02-28 Thread John Keeping 
Using a numbered list is confusing because the exit codes are not listed
in order so the numbers at the start of each line do not match the exit
codes described by the following text.  Switch to a bulleted list so
that the only number appearing on each line is the exit code described.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 Documentation/git-config.txt | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2a04e87..e9c755f 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -58,13 +58,13 @@ that location (you can say '--local' but that is the 
default).
 This command will fail with non-zero status upon error.  Some exit
 codes are:
 
-. The config file is invalid (ret=3),
-. can not write to the config file (ret=4),
-. no section or name was provided (ret=2),
-. the section or key is invalid (ret=1),
-. you try to unset an option which does not exist (ret=5),
-. you try to unset/set an option for which multiple lines match (ret=5), or
-. you try to use an invalid regexp (ret=6).
+- The config file is invalid (ret=3),
+- can not write to the config file (ret=4),
+- no section or name was provided (ret=2),
+- the section or key is invalid (ret=1),
+- you try to unset an option which does not exist (ret=5),
+- you try to unset/set an option for which multiple lines match (ret=5), or
+- you try to use an invalid regexp (ret=6).
 
 On success, the command returns the exit code 0.
 
-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] Re: git config --get-urlmatch does not set exit code 1 when no match is found

2016-02-28 Thread John Keeping 
On Sun, Feb 28, 2016 at 10:45:57AM +, John Keeping wrote:
> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
> the key isn't found, but git-config(1) isn't entirely clear.  The
> overall documentation on exit codes at the end of DESCRIPTION says that
> exit code 1 means:
> 
>   the section or key is invalid (ret=1)
> 
> Then the documentation for the --get option says:
> 
>   Returns error code 1 if the key was not found.
> 
> and --get-all says:
> 
>   Like get, but does not fail if the number of values for the key
>   is not exactly one.
> 
> although it does return 1 if there are zero values.  --get-regexp
> behaves in the same way.
> 
> Overall I think that the fact that --get-urlmatch is the outlier here
> means that it should change to match the other --get* options (ignoring
> --get-color and --get-colorbool which are very different).  Although I
> wonder if anyone is relying on the current behaviour and will find their
> workflow broken if we change this.
> 
> The documentation could also use some clarification since most of the
> return codes only apply for the "set" options and in some cases this
> isn't clear from the existing descriptions.

Here's a series that changes the behaviour of "git config --get-urlmatch"
when no appropriate key is found as well as a couple of improvements to
the documentation while we're here.

The second two patches are independent of the first and I think they
should be picked up even if we decide the change to --get-urlmatch's
exit code is not desirable.

John Keeping (3):
  config: fail if --get-urlmatch finds no value
  Documentation/git-config: use bulleted list for exit codes
  Documentation/git-config: fix --get-all description

 Documentation/git-config.txt | 19 +--
 builtin/config.c |  5 -
 t/t1300-repo-config.sh   |  3 +++
 3 files changed, 16 insertions(+), 11 deletions(-)

-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] config: fail if --get-urlmatch finds no value

2016-02-28 Thread John Keeping 
The --get, --get-all and --get-regexp options to git-config exit with
status 1 if the key is not found but --get-urlmatch succeeds in this
case.

Change --get-urlmatch to behave in the same way as the other --get*
options so that all four are consistent.  --get-color is a special case
because it accepts a default value to return and so should not return an
error if the key is not found.

Also clarify this behaviour in the documentation.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 Documentation/git-config.txt | 2 +-
 builtin/config.c | 5 -
 t/t1300-repo-config.sh   | 3 +++
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 153b2d8..2a04e87 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -102,7 +102,7 @@ OPTIONS
given URL is returned (if no such key exists, the value for
section.key is used as a fallback).  When given just the
section as name, do so for all the keys in the section and
-   list them.
+   list them.  Returns error code 1 if no value is found.
 
 --global::
For writing options: write to global `~/.gitconfig` file
diff --git a/builtin/config.c b/builtin/config.c
index ca9f834..1d7c6ef 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -417,6 +417,7 @@ static int urlmatch_collect_fn(const char *var, const char 
*value, void *cb)
 
 static int get_urlmatch(const char *var, const char *url)
 {
+   int ret;
char *section_tail;
struct string_list_item *item;
struct urlmatch_config config = { STRING_LIST_INIT_DUP };
@@ -443,6 +444,8 @@ static int get_urlmatch(const char *var, const char *url)
git_config_with_options(urlmatch_config_entry, ,
_config_source, respect_includes);
 
+   ret = !values.nr;
+
for_each_string_list_item(item, ) {
struct urlmatch_current_candidate_value *matched = item->util;
struct strbuf buf = STRBUF_INIT;
@@ -459,7 +462,7 @@ static int get_urlmatch(const char *var, const char *url)
free(config.url.url);
 
free((void *)config.section);
-   return 0;
+   return ret;
 }
 
 static char *default_user_config(void)
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 8867ce1..89d8c47 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -1148,6 +1148,9 @@ test_expect_success 'urlmatch' '
cookieFile = /tmp/cookie.txt
EOF
 
+   test_expect_code 1 git config --bool --get-urlmatch doesnt.exist 
https://good.example.com >actual &&
+   test_must_be_empty actual &&
+
echo true >expect &&
git config --bool --get-urlmatch http.SSLverify 
https://good.example.com >actual &&
test_cmp expect actual &&
-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git config --get-urlmatch does not set exit code 1 when no match is found

2016-02-28 Thread John Keeping 
On Sun, Feb 28, 2016 at 10:45:57AM +, John Keeping wrote:
> On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:
> > My current woes are with multi-valued configuration values. More
> > specifically credential.helper
> > 
> > The documentation of git config says that when a value is not matched
> > it should return 1.
> > 
> > To reproduce make sure that credential.helper is not set.
> > 
> > git config --get-urlmatch credential.helper http://somedomain:1234/
> > echo %ERRORLEVEL%
> > 0
> > 
> > git config --get credential.helper
> > echo %ERRORLEVEL%
> > 1
> > 
> > git config --get credential.http://somedomain:1234/.helper
> > echo %ERRORLEVEL%
> > 1
> > 
> > The documentation says that for credential.helper is not found for a
> > domain it should fall back to credential.helper if it is set. So I
> > think that all those tests above should have returned 0. Am i right?

I misread this as "should have returned 1", which is what the text below
agrees with.

The "git config" command does not know anything about the semantics of
particular config keys.  It is purely an interface to parse and query
the config file format and it is up to the consumer to know what to do
if a key doesn't exist.

Both of the "git config --get" examples you give are behaving as
documented in git-config(1).

> It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
> the key isn't found, but git-config(1) isn't entirely clear.  The
> overall documentation on exit codes at the end of DESCRIPTION says that
> exit code 1 means:
> 
>   the section or key is invalid (ret=1)
> 
> Then the documentation for the --get option says:
> 
>   Returns error code 1 if the key was not found.
> 
> and --get-all says:
> 
>   Like get, but does not fail if the number of values for the key
>   is not exactly one.
> 
> although it does return 1 if there are zero values.  --get-regexp
> behaves in the same way.
> 
> Overall I think that the fact that --get-urlmatch is the outlier here
> means that it should change to match the other --get* options (ignoring
> --get-color and --get-colorbool which are very different).  Although I
> wonder if anyone is relying on the current behaviour and will find their
> workflow broken if we change this.
> 
> The documentation could also use some clarification since most of the
> return codes only apply for the "set" options and in some cases this
> isn't clear from the existing descriptions.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git config --get-urlmatch does not set exit code 1 when no match is found

2016-02-28 Thread John Keeping 
On Sun, Feb 28, 2016 at 10:09:12AM +0530, Guilherme wrote:
> My current woes are with multi-valued configuration values. More
> specifically credential.helper
> 
> The documentation of git config says that when a value is not matched
> it should return 1.
> 
> To reproduce make sure that credential.helper is not set.
> 
> git config --get-urlmatch credential.helper http://somedomain:1234/
> echo %ERRORLEVEL%
> 0
> 
> git config --get credential.helper
> echo %ERRORLEVEL%
> 1
> 
> git config --get credential.http://somedomain:1234/.helper
> echo %ERRORLEVEL%
> 1
> 
> The documentation says that for credential.helper is not found for a
> domain it should fall back to credential.helper if it is set. So I
> think that all those tests above should have returned 0. Am i right?

It looks to me like a simple bug that --get-urlmatch doesn't return 1 if
the key isn't found, but git-config(1) isn't entirely clear.  The
overall documentation on exit codes at the end of DESCRIPTION says that
exit code 1 means:

the section or key is invalid (ret=1)

Then the documentation for the --get option says:

Returns error code 1 if the key was not found.

and --get-all says:

Like get, but does not fail if the number of values for the key
is not exactly one.

although it does return 1 if there are zero values.  --get-regexp
behaves in the same way.

Overall I think that the fact that --get-urlmatch is the outlier here
means that it should change to match the other --get* options (ignoring
--get-color and --get-colorbool which are very different).  Although I
wonder if anyone is relying on the current behaviour and will find their
workflow broken if we change this.

The documentation could also use some clarification since most of the
return codes only apply for the "set" options and in some cases this
isn't clear from the existing descriptions.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Revert "rev-parse: remove restrictions on some options"

2016-02-27 Thread John Keeping 
On Fri, Feb 26, 2016 at 06:29:57PM -0500, Jeff King wrote:
> This reverts commit 68889b416d5b6a5cf7d280a428281d635fe9b292.
[snip]
> The original patch was not spurred by an actual bug report,
> but by an observation[1] that was essentially "eh, this
> looks unnecessarily restrictive". It _is_ restrictive, but
> it turns out to be necessarily so. :)

The aim of the original series was to improve the documentation, so I
don't think it's unreasonable to consider this a regression and revert
the functional change.  Although I think we can improve the behaviour
slightly (see below).

> diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
> index b6c6326..0f2bb9b 100644
> --- a/Documentation/git-rev-parse.txt
> +++ b/Documentation/git-rev-parse.txt
> @@ -28,7 +28,8 @@ OPTIONS
>  Operation Modes
>  ~~~
>  
> -Each of these options must appear first on the command line.
> +Each of these options must appear first on the command line; they do not
> +need to be run in a git repository.
>  
>  --parseopt::
>   Use 'git rev-parse' in option parsing mode (see PARSEOPT section below).
> @@ -38,6 +39,18 @@ Each of these options must appear first on the command 
> line.
>   section below). In contrast to the `--sq` option below, this
>   mode does only quoting. Nothing else is done to command input.
>  
> +--local-env-vars::
> + List the GIT_* environment variables that are local to the
> + repository (e.g. GIT_DIR or GIT_WORK_TREE, but not GIT_EDITOR).
> + Only the names of the variables are listed, not their value,
> + even if they are set.

I think we should add:

No other arguments may be supplied.

> +--resolve-git-dir ::
> + Check if  is a valid repository or a gitfile that
> + points at a valid repository, and print the location of the
> + repository.  If  is a gitfile then the resolved path
> + to the real repository is printed.

Again I think this should say that only the `path` argument may be
supplied.

>  --git-path ::
>   Resolve "$GIT_DIR/" and takes other path relocation
>   variables such as $GIT_OBJECT_DIRECTORY,
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index cf8487b..ccc0328 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -518,6 +518,21 @@ int cmd_rev_parse(int argc, const char **argv, const 
> char *prefix)
>   if (argc > 1 && !strcmp("--sq-quote", argv[1]))
>   return cmd_sq_quote(argc - 2, argv + 2);
>  
> + if (argc == 2 && !strcmp("--local-env-vars", argv[1])) {

Maybe:

if (argc > 1 && !strcmp("--local-env-vars", argv[1])) {
if (argc != 2)
die("--local-env-vars must be the only argument");

since the behaviour of:

$ git rev-parse --local-env-vars --
--local-env-vars
--

is quite surprising.

> + int i;
> + for (i = 0; local_repo_env[i]; i++)
> + printf("%s\n", local_repo_env[i]);
> + return 0;
> + }
> +
> + if (argc > 2 && !strcmp(argv[1], "--resolve-git-dir")) {

This is less bad, but again it might be nice to provide a better error
if the path argument isn't supplied.

> + const char *gitdir = resolve_gitdir(argv[2]);
> + if (!gitdir)
> + die("not a gitdir '%s'", argv[2]);
> + puts(gitdir);
> + return 0;
> + }
> +
>   if (argc > 1 && !strcmp("-h", argv[1]))
>   usage(builtin_rev_parse_usage);
>  
> @@ -706,12 +721,6 @@ int cmd_rev_parse(int argc, const char **argv, const 
> char *prefix)
>   add_ref_exclusion(_excludes, arg + 10);
>   continue;
>   }
> - if (!strcmp(arg, "--local-env-vars")) {

What about leaving this in and replacing the body of the if statement
with:

die("--local-env-vars must be the first argument");

?  I expect this will significantly reduce debugging time if anyone is
relying on the current behaviour.

> - int i;
> - for (i = 0; local_repo_env[i]; i++)
> - printf("%s\n", local_repo_env[i]);
> - continue;
> - }
>   if (!strcmp(arg, "--show-toplevel")) {
>   const char *work_tree = get_git_work_tree();
>   if (work_tree)
> @@ -767,16 +776,6 @@ int cmd_rev_parse(int argc, const char **argv, const 
> char *prefix)
>   puts(prefix_filename(pfx, strlen(pfx), 
> get_git_common_dir()));
>   continue;
>   }
> - if (!strcmp(arg, "--resolve-git-dir")) {
> - const char *gitdir = argv[++i];
> - if (!gitdir)
> -

Re: [PATCH 1/2] t8005: avoid grep on non-ASCII data

2016-02-24 Thread John Keeping 
On Tue, Feb 23, 2016 at 03:01:43PM -0800, Junio C Hamano wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> > My original sed version was:
> >
> > sed -ne "/^author /p" -e "/^summary /p"
> >
> > which I think will work on all platforms (we already use it in
> > t-basic.sh) but then I decided to be too clever :-(
> >
> > I still think sed is simpler than introducing a new function to wrap a
> > perl script.
> 
> Let's do this, before everybody forgets what we discussed.

Thanks, this looks good to me.

> -- >8 --
> From: John Keeping <j...@keeping.me.uk>
> Date: Sun, 21 Feb 2016 17:32:21 +
> Subject: [PATCH] t8005: avoid grep on non-ASCII data
> 
> GNU grep 2.23 detects the input used in this test as binary data so it
> does not work for extracting lines from a file.  We could add the "-a"
> option to force grep to treat the input as text, but not all
> implementations support that.  Instead, use sed to extract the desired
> lines since it will always treat its input as text.
> 
> While touching these lines, modernize the test style to avoid hiding the
> exit status of "git blame" and remove a space following a redirection
> operator.  Also swap the order of the expected and actual output
> files given to test_cmp; we compare expect and actual to show how
> actual output differs from what is expected.
> 
> Signed-off-by: John Keeping <j...@keeping.me.uk>
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
>  t/t8005-blame-i18n.sh | 28 
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/t/t8005-blame-i18n.sh b/t/t8005-blame-i18n.sh
> index 847d098..75da219 100755
> --- a/t/t8005-blame-i18n.sh
> +++ b/t/t8005-blame-i18n.sh
> @@ -33,11 +33,15 @@ author $SJIS_NAME
>  summary $SJIS_MSG
>  EOF
>  
> +filter_author_summary () {
> + sed -n -e '/^author /p' -e '/^summary /p' "$@"
> +}
> +
>  test_expect_success !MINGW \
>   'blame respects i18n.commitencoding' '
> - git blame --incremental file | \
> - egrep "^(author|summary) " > actual &&
> - test_cmp actual expected
> + git blame --incremental file >output &&
> + filter_author_summary output >actual &&
> + test_cmp expected actual
>  '
>  
>  cat >expected < @@ -52,9 +56,9 @@ EOF
>  test_expect_success !MINGW \
>   'blame respects i18n.logoutputencoding' '
>   git config i18n.logoutputencoding eucJP &&
> - git blame --incremental file | \
> - egrep "^(author|summary) " > actual &&
> - test_cmp actual expected
> + git blame --incremental file >output &&
> + filter_author_summary output >actual &&
> + test_cmp expected actual
>  '
>  
>  cat >expected < @@ -68,9 +72,9 @@ EOF
>  
>  test_expect_success !MINGW \
>   'blame respects --encoding=UTF-8' '
> - git blame --incremental --encoding=UTF-8 file | \
> - egrep "^(author|summary) " > actual &&
> - test_cmp actual expected
> + git blame --incremental --encoding=UTF-8 file >output &&
> + filter_author_summary output >actual &&
> + test_cmp expected actual
>  '
>  
>  cat >expected < @@ -84,9 +88,9 @@ EOF
>  
>  test_expect_success !MINGW \
>   'blame respects --encoding=none' '
> - git blame --incremental --encoding=none file | \
> - egrep "^(author|summary) " > actual &&
> - test_cmp actual expected
> + git blame --incremental --encoding=none file >output &&
> + filter_author_summary output >actual &&
> + test_cmp expected actual
>  '
>  
>  test_done
> -- 
> 2.7.2-532-g79873b4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git config: do not create .git/ if it does not exist yet

2016-02-24 Thread John Keeping 
On Wed, Feb 24, 2016 at 03:26:57AM -0500, Jeff King wrote:
> On Wed, Feb 24, 2016 at 08:47:00AM +0100, Johannes Schindelin wrote:
> 
> > I cannot think of a way how to test this: all of the regression
> > tests run inside Git's own worktree, and we cannot even assume
> > that /tmp/ is outside of a worktree (or that it exists).
> 
> We could make the test conditional on whether we are in a repo. Anybody
> who builds from a tarball, or who uses --root would then run the test.

Could we use GIT_CEILING_DIRECTORIES for this?  If it's set to
TEST_OUTPUT_DIRECTORY won't that cover the in-tree and out-of-tree test
cases?

We probably do still want Peff's REPOLESS prereq just in case someone is
collecting test results in a repository, but I think that will see much
better coverage than relying on people running tests from the tarball.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git Daemon Dummy: 301 Redirects for git:// to https://

2016-02-23 Thread John Keeping 
On Tue, Feb 23, 2016 at 03:32:02AM +0100, Jason A. Donenfeld wrote:
> In case anyone else finds this useful, I wrote this:
> 
> https://git.zx2c4.com/git-daemon-dummy/about/
> 
> It's an epoll-based responder for git:// that simply returns an error
> telling users of a new URI. The purpose is to phase out git-daemon in
> favor of more secure TLS/HTTPS endpoints. With HTTPS certificates now
> being free, seems like this could be useful.
> 
> My personal motivation is that I'd like to just totally kill the
> git-daemon service, but somebody hard coded a URI of mine into a real
> printed textbook [1], so I don't want it to go stale suddenly. So, I
> need some way of informing users of the new URI.
> 
> Let me know what you think.

There's no license specified in the repo, it just says "All rights
reserved" in the .c file.  I'm sure you intend it to be open source, but
it isn't unless a license is specified.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] t9200: avoid grep on non-ASCII data

2016-02-21 Thread John Keeping 
On Sun, Feb 21, 2016 at 04:15:31PM -0500, Eric Sunshine wrote:
> On Sun, Feb 21, 2016 at 12:32 PM, John Keeping <j...@keeping.me.uk> wrote:
> > GNU grep 2.23 detects the input used in this test as binary data so it
> > does not work for extracting lines from a file.  We could add the "-a"
> > option to force grep to treat the input as text, but not all
> > implementations support that.  Instead, use sed to extract the desired
> > lines since it will always treat its input as text.
> >
> > Signed-off-by: John Keeping <j...@keeping.me.uk>
> > ---
> > diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
> > @@ -35,7 +35,7 @@ exit 1
> >  check_entries () {
> > # $1 == directory, $2 == expected
> > -   grep '^/' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
> > +   sed -ne '\!^/!p' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
> 
> This works with BSD sed, but double negatives are confusing. Have you
> considered this instead?
> 
> sed -ne '/^\//p' ...

What do you mean double negatives?  Do you mean using "!" as an
alternative delimiter?  I find changing delimters is normally simpler
than following multiple levels of quoting for escaping slashes, although
in this case it's simple enough that it doesn't make much difference.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] t8005: avoid grep on non-ASCII data

2016-02-21 Thread John Keeping 
On Sun, Feb 21, 2016 at 06:19:14PM -0500, Jeff King wrote:
> On Sun, Feb 21, 2016 at 04:01:27PM -0500, Eric Sunshine wrote:
> 
> > On Sun, Feb 21, 2016 at 12:32 PM, John Keeping <j...@keeping.me.uk> wrote:
> > > GNU grep 2.23 detects the input used in this test as binary data so it
> > > does not work for extracting lines from a file.  We could add the "-a"
> > > option to force grep to treat the input as text, but not all
> > > implementations support that.  Instead, use sed to extract the desired
> > > lines since it will always treat its input as text.
> > >
> > > While touching these lines, modernize the test style to avoid hiding the
> > > exit status of "git blame" and remove a space following a redirection
> > > operator.
> > >
> > > Signed-off-by: John Keeping <j...@keeping.me.uk>
> > > ---
> > > diff --git a/t/t8005-blame-i18n.sh b/t/t8005-blame-i18n.sh
> > > @@ -35,8 +35,8 @@ EOF
> > >  test_expect_success !MINGW \
> > > 'blame respects i18n.commitencoding' '
> > > -   git blame --incremental file | \
> > > -   egrep "^(author|summary) " > actual &&
> > > +   git blame --incremental file >output &&
> > > +   sed -ne "/^\(author\|summary\) /p" output >actual &&
> > 
> > These tests all crash and burn with BSD sed (including Mac OS X) since
> > you're not restricting yourself to BRE (basic regular expressions).
> > You _could_ request extended regular expressions, which do work on
> > those platforms, as well as with GNU sed:
> > 
> > sed -nEe "/^(author|summary) /p" ...
> 
> At that point, I think we may as well use grep, because obscure
> platforms are probably broken either way.

Also GNU sed doesn't understand "-E", it uses "-r" for --regexp-extended.

> I'm tempted to just go the perl route. We already depend on at least a
> baisc version of perl5 being installed for many of the other tests, so
> it's not really introducing a new dependency.
> 
> Something like the patch below works for me. I think we could make it
> shorter by using $PERLIO to get the raw behavior, but using binmode will
> work even on ancient versions of perl.
> 
> John, if you agree on the direction, feel free to combine it with your
> patch.

My original sed version was:

sed -ne "/^author /p" -e "/^summary /p"

which I think will work on all platforms (we already use it in
t-basic.sh) but then I decided to be too clever :-(

I still think sed is simpler than introducing a new function to wrap a
perl script.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] t8005: avoid grep on non-ASCII data

2016-02-21 Thread John Keeping 
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file.  We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that.  Instead, use sed to extract the desired
lines since it will always treat its input as text.

While touching these lines, modernize the test style to avoid hiding the
exit status of "git blame" and remove a space following a redirection
operator.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 t/t8005-blame-i18n.sh | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/t/t8005-blame-i18n.sh b/t/t8005-blame-i18n.sh
index 847d098..0a86c72 100755
--- a/t/t8005-blame-i18n.sh
+++ b/t/t8005-blame-i18n.sh
@@ -35,8 +35,8 @@ EOF
 
 test_expect_success !MINGW \
'blame respects i18n.commitencoding' '
-   git blame --incremental file | \
-   egrep "^(author|summary) " > actual &&
+   git blame --incremental file >output &&
+   sed -ne "/^\(author\|summary\) /p" output >actual &&
test_cmp actual expected
 '
 
@@ -52,8 +52,8 @@ EOF
 test_expect_success !MINGW \
'blame respects i18n.logoutputencoding' '
git config i18n.logoutputencoding eucJP &&
-   git blame --incremental file | \
-   egrep "^(author|summary) " > actual &&
+   git blame --incremental file >output &&
+   sed -ne "/^\(author\|summary\) /p" output >actual &&
test_cmp actual expected
 '
 
@@ -68,8 +68,8 @@ EOF
 
 test_expect_success !MINGW \
'blame respects --encoding=UTF-8' '
-   git blame --incremental --encoding=UTF-8 file | \
-   egrep "^(author|summary) " > actual &&
+   git blame --incremental --encoding=UTF-8 file >output &&
+   sed -ne "/^\(author\|summary\) /p" output >actual &&
test_cmp actual expected
 '
 
@@ -84,8 +84,8 @@ EOF
 
 test_expect_success !MINGW \
'blame respects --encoding=none' '
-   git blame --incremental --encoding=none file | \
-   egrep "^(author|summary) " > actual &&
+   git blame --incremental --encoding=none file >output &&
+   sed -ne "/^\(author\|summary\) /p" output >actual &&
test_cmp actual expected
 '
 
-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] t9200: avoid grep on non-ASCII data

2016-02-21 Thread John Keeping 
GNU grep 2.23 detects the input used in this test as binary data so it
does not work for extracting lines from a file.  We could add the "-a"
option to force grep to treat the input as text, but not all
implementations support that.  Instead, use sed to extract the desired
lines since it will always treat its input as text.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 t/t9200-git-cvsexportcommit.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 812c9cd..0765d52 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -35,7 +35,7 @@ exit 1
 
 check_entries () {
# $1 == directory, $2 == expected
-   grep '^/' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
+   sed -ne '\!^/!p' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
if test -z "$2"
then
>expected
-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] Fix test failures with GNU grep 2.23

2016-02-21 Thread John Keeping 
On Fri, Feb 19, 2016 at 02:33:10PM -0500, Jeff King wrote:
> On Fri, Feb 19, 2016 at 07:23:11PM +0000, John Keeping wrote:
> 
> > I suspect that any grep that lacks "-a" also lacks binary file handling
> > that will break these tests.  I found a Solaris grep that doesn't
> > support "-a" and it treats these files as text.
> > 
> > From that perspective, it would be better to have a central place that
> > deals with figuring out how to get grep to work for us.  Perhaps we need
> > test_grep to get this right.  We already have test_cmp_bin() as a thin
> > wrapper around cmp so I don't think this is completely unprecedented.
> 
> I think 99% of the time we are using grep for ascii text. As evidenced
> by the number of test failures we see with the new grep, it is a small
> minority that feed binary gibberish. I'd prefer if "-a" handling didn't
> need to pollute anything outside of this narrow range of tests (and as
> with my prereq suggestion, I am even find just skipping this narrow
> range of tests on platforms with no "-a", though falling back to running
> without "-a" is fine if it works).

I went with using sed in this series because it seems to be the simplest
and most compatible way to extract lines from the input.  We don't need
any special casing to figure out if an implementation needs "-a" or if
it doesn't support that option and all the implementation I tested
support the constructs used here.

John Keeping (2):
  t8005: avoid grep on non-ASCII data
  t9200: avoid grep on non-ASCII data

 t/t8005-blame-i18n.sh  | 16 
 t/t9200-git-cvsexportcommit.sh |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

-- 
2.7.1.503.g3cfa3ac

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Test failures with GNU grep 2.23

2016-02-19 Thread John Keeping 
On Fri, Feb 19, 2016 at 09:38:17AM -0800, Junio C Hamano wrote:
> Jeff King  writes:
> 
> > Yeah, I'd worry that "-a" is not portable. OTOH, BSD grep seems to have
> > it, so between that and GNU, I think most systems are covered. We could
> > do:
> >
> >   test_lazy_prereq GREP_A '
> > echo foo | grep -a foo
> >   '
> >
> > and mark these tests with it. I'd also be happy to skip that step and
> > just do it if and when somebody actually complains about a system
> > without it (I wouldn't be surprised if most people on antique systems
> > end up installing GNU grep anyway).
> >
> > Another option might be using "sed -ne '/^author/p'" or similar. But
> > that may very well just be trading one portability problem for another.
> 
> Would $PERL help, I wonder?

I suspect that any grep that lacks "-a" also lacks binary file handling
that will break these tests.  I found a Solaris grep that doesn't
support "-a" and it treats these files as text.

>From that perspective, it would be better to have a central place that
deals with figuring out how to get grep to work for us.  Perhaps we need
test_grep to get this right.  We already have test_cmp_bin() as a thin
wrapper around cmp so I don't think this is completely unprecedented.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Timezone with DATE_STRFTIME

2016-02-08 Thread John Keeping 
On Mon, Feb 08, 2016 at 10:28:58AM -0500, Jeff King wrote:
> On Mon, Feb 08, 2016 at 02:33:17PM +0000, John Keeping wrote:
> 
> > I have just noticed that with DATE_STRFTIME, the timezone in the output
> > is likely to be incorrect.
> > 
> > For all other time formats, we print the string ourselves and use the
> > correct timezone from the input, but with DATE_STRFTIME strftime(3) will
> > always use the system timezone.
> 
> You mean here that the "%z" formatting will not be correct, right?
> AFAICT the time shown is generally correct for the original of the
> author, and we simply need to communicate the zone to strftime.
> 
> Taking the current tip of master, for instance, I get:
> 
>   $ for i in \
>   default \
>   local \
>   "format:%H:%M %z" \
>   "format-local:%H:%M %z"; do
> git log -1 --format=%ad --date="$i" ff4ea6004
> done
>   Fri Feb 5 15:24:02 2016 -0800
>   Fri Feb 5 18:24:02 2016
>   15:24 +
>   18:24 +
> 
> You can see that my system is in -0500, three hours ahead of the author.
> And as expected, strftime shows the time in the original author's
> timezone. The %z information is totally bogus, but I don't think it has
> anything to do with the system time. It is simply that we don't provide
> it (...but having just looked at _your_ local timezone from your email,
> I can guess how you got confused :) ).
> 
> So I think the fix is probably just that we need to feed the zone
> information to strftime via the "struct tm".

If "struct tm" had a standard field for that...

Obviously "struct tm" does have a field for the offset (which is how we
end up in + above, because our "struct tm" comes from gmtime(3)),
but it's not standardized so I don't think we can rely on it.

AFAICT the only way to pass the timezone into the C library time
functions is via $TZ or the global "timezone" variable, but from looking
at a couple of implementations I don't think strftime() will actually
look at those (the timezone is instead embedded when the "struct tm" is
generated).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Timezone with DATE_STRFTIME

2016-02-08 Thread John Keeping 
I have just noticed that with DATE_STRFTIME, the timezone in the output
is likely to be incorrect.

For all other time formats, we print the string ourselves and use the
correct timezone from the input, but with DATE_STRFTIME strftime(3) will
always use the system timezone.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Test failures with GNU grep 2.23

2016-02-07 Thread John Keeping 
It seems that binary file detection has changed in GNU grep 2.23 as a
result of commit 40ed879 (grep: fix bug with with invalid unibyte
sequence).

This causes a couple of test failures in t8005 and t9200 (the t9200 case
is less obvious so I'm only including t8005 here):

-- >8 --
$ ./t8005-blame-i18n.sh -v -i
[snip]
expecting success: 
git blame --incremental file | \
egrep "^(author|summary) " > actual &&
test_cmp actual expected

--- actual  2016-02-07 16:14:55.372510307 +
+++ expected2016-02-07 16:14:55.359510341 +
@@ -1 +1,6 @@
-Binary file (standard input) matches
+author �R�c ���Y
+summary �u���[���̃e�X�g�ł��B
+author �R�c ���Y
+summary �u���[���̃e�X�g�ł��B
+author �R�c ���Y
+summary �u���[���̃e�X�g�ł��B
not ok 2 - blame respects i18n.commitencoding
#
#   git blame --incremental file | \
#   egrep "^(author|summary) " > actual &&
#   test_cmp actual expected
#
-- 8< --

The following patch fixes the tests for me, but I wonder if "-a" is
supported on all target platforms (it's not in POSIX, which specifies
that the "input files shall be text files") or whether we should do
something more comprehensive to provide sane_{e,f,}grep which guarantee
to treat input as text.

I also tried setting POSIXLY_CORRECT but that doesn't affect the
text/binary decision.

-- >8 --
diff --git a/t/t8005-blame-i18n.sh b/t/t8005-blame-i18n.sh
index 847d098..3b6e697 100755
--- a/t/t8005-blame-i18n.sh
+++ b/t/t8005-blame-i18n.sh
@@ -36,7 +36,7 @@ EOF
 test_expect_success !MINGW \
'blame respects i18n.commitencoding' '
git blame --incremental file | \
-   egrep "^(author|summary) " > actual &&
+   egrep -a "^(author|summary) " > actual &&
test_cmp actual expected
 '
 
@@ -53,7 +53,7 @@ test_expect_success !MINGW \
'blame respects i18n.logoutputencoding' '
git config i18n.logoutputencoding eucJP &&
git blame --incremental file | \
-   egrep "^(author|summary) " > actual &&
+   egrep -a "^(author|summary) " > actual &&
test_cmp actual expected
 '
 
@@ -69,7 +69,7 @@ EOF
 test_expect_success !MINGW \
'blame respects --encoding=UTF-8' '
git blame --incremental --encoding=UTF-8 file | \
-   egrep "^(author|summary) " > actual &&
+   egrep -a "^(author|summary) " > actual &&
test_cmp actual expected
 '
 
@@ -85,7 +85,7 @@ EOF
 test_expect_success !MINGW \
'blame respects --encoding=none' '
git blame --incremental --encoding=none file | \
-   egrep "^(author|summary) " > actual &&
+   egrep -a "^(author|summary) " > actual &&
test_cmp actual expected
 '
 
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 5cfb9cf..f05578a 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -35,7 +35,7 @@ exit 1
 
 check_entries () {
# $1 == directory, $2 == expected
-   grep '^/' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
+   grep -a '^/' "$1/CVS/Entries" | sort | cut -d/ -f2,3,5 >actual
if test -z "$2"
then
>expected
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: no luck with colors for branch names in gitk yet

2016-02-05 Thread John Keeping 
On Fri, Feb 05, 2016 at 01:29:26PM -0900, Britton Kerin wrote:
> On Fri, Feb 5, 2016 at 12:25 PM, Philip Oakley  wrote:
> > From: "Britton Kerin" 
> >>
> >> Someone suggested using color.branch.upstream, I tried like this and
> >> variants
> >>
> >> [color "branch"]
> >>  local = red bold
> >>  upstream = red bold
> >>
> >> Doesn't seem to matter what I put in for upstream, including invalid
> >> colors, gitk just ignores it and does the dark green for local
> >> branches
> >> --
> >
> > Alternate, try
> > https://github.com/oumu/mintty-color-schemes/blob/master/base16-mintty/base16-default.minttyrc
> > (or any of the other colour schemes) and copy them into your .minttyrc file
> > (works for me on g4w : git version 2.7.0.windows.1 )
> 
> I'm on linux so I think mintty is not an option.  Also, I'm a little
> surprised in affects the rendering of branch tags in gitk, I would
> have thought that would be an X or window system thing.

I think Philip missed that you were talking about gitk.  It seems that
the problem comes from updating to Tcl/Tk 7.6, which makes green darker
as described in commit 66db14c (gitk: Color name update, 2015-10-25) and
by TIP #403 [1].

However, it seems that gitk won't actually use the updated colour if you
have an existing ~/.gitk file.  You can just replace "green" with "lime"
in that file to get the new defaults, but I wonder if we should force
that for users who already have the previous defaults saved.


[1] http://www.tcl.tk/cgi-bin/tct/tip/403.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping 
On Thu, Feb 04, 2016 at 01:01:39PM +0100, Johannes Schindelin wrote:
> I noticed through a nearby patch series that was submitted by Elia that
> some of the $((...)) expressions introduced in scripts that I introduced
> to Git's source code did not match the existing code's convention:
> previously these expressions did not contain any spaces, now *some* do.
> 
> This patch series tries to clean that up quickly before even more code
> has to decide which one of the disagreeing coding conventions to use.
> 
> Note: For the sake of getting this patch series out, I skipped t/ and
> contrib/. I do not care much about the latter, but t/ should probably be
> fixed, too.

Should this be going this other way (i.e. standardising on having the
spaces)?

The current state (excluding contrib/ and t/) seems to favour spaces:

$ git grep '\$((' -- ':/' ':!t/' ':!contrib/'
Documentation/CodingGuidelines: - We use Arithmetic Expansion $(( ... )).
Documentation/CodingGuidelines:   of them, as some shells do not grok $((x)) 
while accepting $(($x))
generate-cmdlist.sh:n=$(($n+1))
git-filter-branch.sh:   elapsed=$(($now - $start_timestamp))
git-filter-branch.sh:   remaining=$(( ($commits - $count) * $elapsed / 
$count ))
git-filter-branch.sh:   next_sample_at=$(( ($elapsed + 1) * 
$count / $elapsed ))
git-filter-branch.sh:   next_sample_at=$(($next_sample_at + 1))
git-filter-branch.sh:   
git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))
git-rebase--interactive.sh: total=$(($new_count + $(git stripspace 
--strip-comments <"$todo" | wc -l)))
git-rebase--interactive.sh: count=$(($(sed -n \
git-rebase--interactive.sh: lineno=$(( $lineno + 1 ))
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-rebase--merge.sh:   eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - 
$msgnum))"'
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-rebase--merge.sh:   msgnum=$(($msgnum + 1))
git-submodule.sh:   n=$(($1 + 0)) 2>/dev/null && test "$n" = "$1"
git-submodule.sh:   total_commits=" ($(($total_commits + 
0)))"

I make that 3 without spaces (including the git-rebase--interactive.sh
case that wraps) and 12 that do have spaces around operators.  Using
spaces around operators also matches our C coding style.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping 
On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Although I don't think the historic context is useful in deciding which
> > direction to go in the future.
> 
> Being a maintainer, I find that argument particularly hard to defend.

I worded that badly, what I wanted to say is that how we got here is
less interesting than where we are.  From a quick bit of grep'ing it
looks to me like where we are is in favour of adding spaces around
binary operators inside $(( )) constructs based on the majority of the
uses in the code as it currently stands.

> But sure, you go ahead and prepare a patch series that turns everything
> around, adding spaces around those operators.
> 
> Whatever the outcome, the inconsistency must be fixed.

I disagree.  Unless there are other changes in the same area, the noise
isn't worth it.

However, I do think we need to agree on a policy so that new code can be
consistent.  This should then be documented in CodingGuidelines.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping 
On Thu, Feb 04, 2016 at 01:38:51PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > Using spaces around operators also matches our C coding style.
> 
> Well, by that reasoning you should go the whole nine yards and write
> 
>   lineno = $(( $lineno + 1 ))
> 
> Except you can't. Because shell code is inherently not like C code.

That isn't my main argument, although I do think the (presumed)
rationale for using spaces in C to improve usability applies here as
well, even if the confines of the language don't let us go as far as in
C.

I'm not actually sure whether spaces inside the enclosing $(( and )) are
helpful, we're much less consistent about that than about spaces around
binary operators.  Having looked at t/ as well now, we really do seem to
favour using spaces around the binary operators so I'm further convinced
this series is switching the wrong cases.

> What I found particularly interesting about 180bad3 (rebase -i: respect
> core.commentchar, 2013-02-11) was that it *snuck in* that coding style: it
> *changed* the existing one (without rationale in the commit message, too).

The last version of that patch I can find in the archive [1] doesn't add
the spaces, so I think they must have been part of Junio's fixup
discusses in the following messages.  Although I don't think the
historic context is useful in deciding which direction to go in the
future.

[1] http://article.gmane.org/gmane.comp.version-control.git/216103
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] Fix $((...)) coding style

2016-02-04 Thread John Keeping 
On Thu, Feb 04, 2016 at 04:27:49PM +0100, Johannes Schindelin wrote:
> On Thu, 4 Feb 2016, John Keeping wrote:
> 
> > On Thu, Feb 04, 2016 at 02:13:47PM +0100, Johannes Schindelin wrote:
> > > Whatever the outcome, the inconsistency must be fixed.
> > 
> > I disagree.  Unless there are other changes in the same area, the noise
> > isn't worth it.
> > 
> > However, I do think we need to agree on a policy so that new code can be
> > consistent.  This should then be documented in CodingGuidelines.
> 
> If you want to enforce it in the future, how can you possibly be against
> fixing the inconsistency in the existing code? Sorry, I am really unable
> to understand this.

I avoided quoting CodingGuidelines in my previous message, but it says:

 - Fixing style violations while working on a real change as a
   preparatory clean-up step is good, but otherwise avoid useless code
   churn for the sake of conforming to the style.

   "Once it _is_ in the tree, it's not really worth the patch noise to
   go and fix it up."
   Cf. http://article.gmane.org/gmane.linux.kernel/943020

> Also, we *did* document the policy in the CodingGuidelines:
> 
>   As for more concrete guidelines, just imitate the existing code
> 
> So. There you are. By that token, my patch series was the correct thing to
> do because the first arithmetic expression introduced into a shell script
> in Git's source code had no spaces.

This is the first point I made.  To take one example, in
git-filter-branch.sh there are five occurrences of the sequence "$((";
your patch changes four of them to remove spaces.  If we standardise on
having spaces only one needs to change:

$ git grep -F '$((' origin/master -- git-filter-branch.sh
origin/master:git-filter-branch.sh: elapsed=$(($now - 
$start_timestamp))
origin/master:git-filter-branch.sh: remaining=$(( ($commits - 
$count) * $elapsed / $count ))
origin/master:git-filter-branch.sh: next_sample_at=$(( 
($elapsed + 1) * $count / $elapsed ))
origin/master:git-filter-branch.sh: 
next_sample_at=$(($next_sample_at + 1))
origin/master:git-filter-branch.sh: 
git_filter_branch__commit_count=$(($git_filter_branch__commit_count+1))

I chose git-filter-branch.sh simply because it was touched by this patch
set but it is not an outlier in this regard.  Some crude statistics
across all of git.git:

# No space after plus
$ git grep -F '$((' | grep '\+[\$0-9]' | wc -l
34

# With space after plus
$ git grep -F '$((' | grep '\+ [\$0-9]' | wc -l
96
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] completion: verify-tag is not plumbing

2016-02-01 Thread John Keeping 
On Sun, Jan 31, 2016 at 02:37:59PM +0100, SZEDER Gábor wrote:
> 
> Quoting John Keeping <j...@keeping.me.uk>:
> 
> > According to command-list.txt, verify-tag is an ancillary interrogator,
> > which means that it should be completed by "git verify-" in the
> > same way as verify-commit.
> >
> > Remove it from the list of plumbing commands so that it is treated as
> > porcelain and completed.
> 
> I'm not sure.  There are commands among the ancillary interrogators
> that are basically porcelains (e.g. blame), while some are more like
> plumbing (e.g. rerere, rev-parse).  In general the completion script
> supports the former but not the latter commands.
> 
> Now, the real porcelain-ish way to verify a tag is via 'git tag
> -v|--verify', and according to a925c6f165a3 (bash: Classify more
> commends out of completion., 2007-02-04), the commit removing
> verify-tag from the completed commands, verify-tag was kept around for
> backwards compatibility reasons.  OTOH verify-commit was introduced in
> d07b00b7f31d (verify-commit: scriptable commit signature verification,
> 2014-06-23), and as the subject line states it was intended more as a
> plumbing command.
> 
> So I think we should keep excluding verify-tag from the list of
> porcelain commands in the completion script, and it was an oversight
> not to exclude verify-commit as well when it was introduced.

I can accept that argument about verify-commit and verify-tag, but
listing verify-tag as plumbing is incorrect according to
command-list.txt (and thus git(1)).  If we're going to classify
commands, shouldn't we be consistent in how we do so?

> > Signed-off-by: John Keeping <j...@keeping.me.uk>
> > ---
> >  contrib/completion/git-completion.bash | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash
> > b/contrib/completion/git-completion.bash
> > index 51f5223..250788a 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
> > write-tree)   : plumbing;;
> > var)  : infrequent;;
> > verify-pack)  : infrequent;;
> > -   verify-tag)   : plumbing;;
> > *) echo $i;;
> > esac
> > done
> > --
> > 2.7.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] completion: verify-tag is not plumbing

2016-01-31 Thread John Keeping 
According to command-list.txt, verify-tag is an ancillary interrogator,
which means that it should be completed by "git verify-" in the
same way as verify-commit.

Remove it from the list of plumbing commands so that it is treated as
porcelain and completed.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 contrib/completion/git-completion.bash | 1 -
 1 file changed, 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 51f5223..250788a 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -728,7 +728,6 @@ __git_list_porcelain_commands ()
write-tree)   : plumbing;;
var)  : infrequent;;
verify-pack)  : infrequent;;
-   verify-tag)   : plumbing;;
*) echo $i;;
esac
done
-- 
2.7.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] completion: add missing git-rebase options

2016-01-21 Thread John Keeping 
This adds the --no-* variants where those are documented in
git-rebase(1).

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 contrib/completion/git-completion.bash | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 482ca84..7d6d63e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1685,8 +1685,12 @@ _git_rebase ()
--preserve-merges --stat --no-stat
--committer-date-is-author-date --ignore-date
--ignore-whitespace --whitespace=
-   --autosquash --fork-point --no-fork-point
-   --autostash
+   --autosquash --no-autosquash
+   --fork-point --no-fork-point
+   --autostash --no-autostash
+   --verify --no-verify
+   --keep-empty --root --force-rebase --no-ff
+   --exec
"
 
return
-- 
2.7.0.226.gfe986fe

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Odd rebase behavior

2015-12-19 Thread John Keeping 
On Fri, Dec 18, 2015 at 06:05:49PM +, John Keeping wrote:
> On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
> > John Keeping <j...@keeping.me.uk> writes:
> > 
> > > It seems that the problem is introduces by --preserve-merges (and
> > > -Xsubtree causes something interesting to happen as well).  I see the
> > > following behaviour:
> > 
> > Thanks for narrowing this down!  Is it possible this is actually a
> > cherry-pick problem since --preserve-merges forces rebase to use
> > cherry-pick?
> 
> I'm pretty sure this a result of the code in git-rebase--interactive.sh
> just below the comment "Watch for commits that have been dropped by
> cherry-pick", which filters out certain commits.  However, I'm not at
> all familiar with the --preserve-merges code in git-rebase so I could be
> completely wrong.

I've traced through git-rebase--interactive.sh and I can see what's
happening here now.  The problematic code is around the handling of the
"rewritten" directory.

In --preserve-merges mode, we write the SHA1 of the "onto" commit into
rewritten and then add any commits descended from it along the
first-parent path that we have identified as candidates for being
rebased.  This allows us to identify commits that have been merged in
and remove them from the rebase instruction list.

Because the right-hand commit in this case is disjoint from "onto", we
end up dropping everything at this point.  The --root option fixes this
because instead of preserving just "onto", it adds all of the commits
given by:

git merge-base --all $orig_head $upstream

Since the disjoint history causes the root commit to be rewritten, I
think requiring --root for this case to work is reasonable.  However,
I'm not sure we should add it automatically.  It may be better to detect
that there is no common history and die if --root has not been given.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Odd rebase behavior

2015-12-18 Thread John Keeping 
On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:
> John Keeping <j...@keeping.me.uk> writes:
> 
> > It seems that the problem is introduces by --preserve-merges (and
> > -Xsubtree causes something interesting to happen as well).  I see the
> > following behaviour:
> 
> Thanks for narrowing this down!  Is it possible this is actually a
> cherry-pick problem since --preserve-merges forces rebase to use
> cherry-pick?

I'm pretty sure this a result of the code in git-rebase--interactive.sh
just below the comment "Watch for commits that have been dropped by
cherry-pick", which filters out certain commits.  However, I'm not at
all familiar with the --preserve-merges code in git-rebase so I could be
completely wrong.

> > git rebase -Xsubtree=files_subtree --onto files-master master
> >
> > fatal: Could not parse object 
> > 'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
> > Unknown exit code (128) from command: git-merge-recursive
> > b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
> > b15c4133fc3146e1330c84159886f0f7a09fbf43
> 
> Ah, good!  I had seen this behavior as well but couldn't remember what I
> did to trigger it.
> 
> I don't think I have the expertise to fix rebase and/or cherry-pick.
> What's the process for adding these tests to the testbase and marking
> them so the appropriate person can fix them?  I see a lot of TODO tests.
> Should I mark these similarly and propose a patch to the testbase?

I think marking them with test_expect_failure (instead of
test_expect_success) is enough.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Odd rebase behavior

2015-12-16 Thread John Keeping 
On Tue, Dec 15, 2015 at 09:17:30PM -0600, David A. Greene wrote:
> According to the rebase man page, rebase gathers commits as in "git log
> ..HEAD."  However, that is not what happens in the tests
> below.  Some of the commits disappear.
> 
> The test basically does this:
> 
> - Setup a master project and a subproject, merged via a subtree-like
>   merge (this is how git-subtree does it).
> 
> - Add some commits to the subproject directory after the subtree merge,
>   to create some history not in the original subproject.
> 
> - filter-branch --subdirectory-filter to extract commits from the
>   subproject directory.
> 
> - Rebase those commits back on to the original subproject repository.
> 
> The above loses all commits made after the subproject is merged into
> the main project.
[snip]
> # Does not preserve master4 and master5.
> test_expect_success 'Rebase default' '
>   git checkout -b rebase-default master &&
>   git filter-branch --prune-empty -f --subdirectory-filter files_subtree 
> &&
>   git commit -m "Empty commit" --allow-empty &&
>   git rebase -Xsubtree=files_subtree  --preserve-merges --onto 
> files-master master &&

It seems that the problem is introduces by --preserve-merges (and
-Xsubtree causes something interesting to happen as well).  I see the
following behaviour:

git rebase --onto files-master master

Works (master4 and master5 preserved).

git rebase --preserve-merges --onto files-master master

Behaves as described above (master4 and master5 are lost).

git rebase -Xsubtree=files_subtree --onto files-master master

fatal: Could not parse object 
'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
Unknown exit code (128) from command: git-merge-recursive 
b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD 
b15c4133fc3146e1330c84159886f0f7a09fbf43 

git rebase -Xsubtree=files_subtree --preserve-merges --onto files-master master

Same as the version with only --preserve-merges.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

2015-12-10 Thread John Keeping 
On Thu, Dec 10, 2015 at 02:46:40PM -0800, Junio C Hamano wrote:
> * jk/send-email-ssl-errors (2015-11-24) 1 commit
>  - send-email: enable SSL level 1 debug output
> 
>  Improve error reporting when SMTP TLS fails.
> 
>  Waiting for a reroll.
>  ($gmane/281693)

It looks like this got lost in the noise:

http://article.gmane.org/gmane.comp.version-control.git/281975
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: best practices against long git rebase times?

2015-12-04 Thread John Keeping 
On Fri, Dec 04, 2015 at 04:05:46PM +0100, Andreas Krey wrote:
> our workflow is pretty rebase-free for diverse reasons yet.
> 
> One obstacle now appearing is that rebases simply take
> very long - once you might want to do a rebase there are
> several hundred commits on the remote branch, and our tree
> isn't small either.
> 
> This produces rebase times in the minute range.
> I suppose this is because rebase tries to see
> if there are new commits in the destination
> branch that are identical to one of the local
> commits, to be able to skip them. (I didn't
> try to verify this hypothesis.)
> 
> What can we do to make this faster?

I'm pretty sure that you're right and the cherry-pick analysis is where
the time is spent.

I looked into this a couple of years ago and I have a variety of
(half-finished) experiments that might improve the performance of this:

https://github.com/johnkeeping/git/commits/log-cherry-no-merges
https://github.com/johnkeeping/git/commits/patch-id-limit-paths

https://github.com/johnkeeping/git/commits/revision-cherry-respect-ancestry-path
https://github.com/johnkeeping/git/commits/patch-id-notes-cache
http://comments.gmane.org/gmane.comp.version-control.git/224006

I have no idea if any of these changes will apply to modern Git (or if
some of them are even correct) but I can try to clean them up if there's
interest.

The commit for patch-id-limit-paths includes some numbers that might be
relevant for your case:

Before:
$ time git log --cherry master...jk/submodule-subdirectory-ok >/dev/null

real0m0.373s
user0m0.341s
sys 0m0.031s

After:
$ time git log --cherry master...jk/submodule-subdirectory-ok >/dev/null

real0m0.060s
user0m0.055s
sys 0m0.005s
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: best practices against long git rebase times?

2015-12-04 Thread John Keeping 
On Fri, Dec 04, 2015 at 06:09:33PM +0100, demerphq wrote:
> On 4 December 2015 at 16:05, Andreas Krey  wrote:
> > Hi all,
> >
> > our workflow is pretty rebase-free for diverse reasons yet.
> >
> > One obstacle now appearing is that rebases simply take
> > very long - once you might want to do a rebase there are
> > several hundred commits on the remote branch, and our tree
> > isn't small either.
> >
> > This produces rebase times in the minute range.
> > I suppose this is because rebase tries to see
> > if there are new commits in the destination
> > branch that are identical to one of the local
> > commits, to be able to skip them. (I didn't
> > try to verify this hypothesis.)
> >
> > What can we do to make this faster?
> 
> I bet you have a lot of refs; tags, or branches.
> 
> git rebase performance along with many operations seems to scale
> proportionately to the number of tags.
> 
> At $work we create a tag every time we "roll out" a "server type".
> 
> This produces many tags a day.
> 
> Over time rebase, and many operations actually, start slowing down to
> the point of painfulness.
> 
> The workaround we ended up using was to set up a cron job and related
> infra that removed old tags.
> 
> Once we got rid of most of our old tags git became nice to use again.

This is quite surprising.  Were you using packed or loose tags?

It would be interesting to run git-rebase with GIT_TRACE_PERFORMANCE to
see which subcommand is slow in this particular scenario.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] send-email: enable SSL level 1 debug output

2015-12-03 Thread John Keeping 
If a server's certificate isn't accepted by send-email, the output is:

Unable to initialize SMTP properly. Check config and use --smtp-debug.

but adding --smtp-debug=1 just produces the same output since we don't
get as far as talking SMTP.

Turning on SSL debug at level 1 gives:

DEBUG: .../IO/Socket/SSL.pm:1796: SSL connect attempt failed 
error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify 
failed
DEBUG: .../IO/Socket/SSL.pm:673: fatal SSL error: SSL connect attempt 
failed error:14090086:SSL routines:ssl3_get_server_certificate:certificate 
verify failed
DEBUG: .../IO/Socket/SSL.pm:1780: IO::Socket::IP configuration failed

IO::Socket::SSL defines level 1 debug as "print out errors from
IO::Socket::SSL and ciphers from Net::SSLeay".  In fact, it aliases
Net::SSLeay::trace which is defined to guarantee silence at level 0 and
only emit error messages at level 1, so let's enable it by default.

The modification of warnings is needed to avoid a warning about:

Name "IO::Socket::SSL::DEBUG" used only once: possible typo

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
Sorry about the delay following up with this.

I don't particularly like the need for brackets and modifying the
warnings here, but AFAIK there is no other way to avoid a warning that
is likely to upset users (although I am far from a Perl expert).

 git-send-email.perl | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index e907e0e..72508be 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1318,6 +1318,13 @@ Message-Id: $message_id
require Net::SMTP::SSL;
$smtp_domain ||= maildomain();
require IO::Socket::SSL;
+
+   # Suppress "variable accessed once" warning.
+   {
+   no warnings 'once';
+   $IO::Socket::SSL::DEBUG = 1;
+   }
+
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Nov 2015, #04; Tue, 24)

2015-11-25 Thread John Keeping 
On Tue, Nov 24, 2015 at 08:07:23PM -0500, Jeff King wrote:
> * jk/send-email-ssl-errors (2015-11-24) 1 commit
>  - send-email: enable SSL level 1 debug output
> 
>  Improve error reporting when SMTP TLS fails.
> 
>  Will merge to 'next'.

Can you hold off on this one?  I think my last-minute change not to
switch on --smtp-debug has introduced a Perl warning that needs to be
suppressed.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] send-email: die if CA path doesn't exist

2015-11-25 Thread John Keeping 
On Tue, Nov 24, 2015 at 06:35:36PM -0500, Jeff King wrote:
> On Tue, Nov 24, 2015 at 11:31:40PM +0000, John Keeping wrote:
> 
> > If the CA path isn't found it's most likely to indicate a
> > misconfiguration, in which case accepting any certificate is unlikely to
> > be the correct thing to do.
> 
> Thanks.
> 
> > Changes since v1:
> > - add missing path to error message
> > - remove trailing '.' on error message since die appends "at
> >   /path/to/git-send-email line ..."
> 
> It won't if the error message ends with a newline. We seem to be wildly
> inconsistent about that in send-email, though.

Interesting.  I think in this case it would definitely be better to add
the newline and avoid printing the location in the script, but it may
make more sense to have a separate pass over git-send-email.perl and fix
all of the die() calls.

I suspect that everything except the equivalent of BUG() should be
suppressing the location in a user-facing script like this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

2015-11-24 Thread John Keeping 
On Tue, Nov 24, 2015 at 02:58:43PM -0500, Jeff King wrote:
> On Fri, Nov 20, 2015 at 07:46:51PM +0000, John Keeping wrote:
> 
> > > For people who know their systems are broken and want to proceed anyway,
> > > what is the appropriate work-around? Obviously it involves disabling
> > > peer verification, but would we want to include instructions for doing
> > > so (either in the error message, or perhaps mentioning it in the commit
> > > message)?
> > 
> > The documentation already says:
> > 
> > Set it to an empty string to disable certificate verification.
> > 
> > It's a bit lost in the middle of a paragraph but I think that is the
> > best place for the detail of how to disable verification.
> > 
> > Having revisted the patch, I do think the message might be a bit terse,
> > but I can't think of a reasonably concise way to point at the
> > --smtp-ssl-cert-path argument as being the culprit.
> 
> Hrm. I was thinking that somebody might not have any clue that
> --smtp-ssl-cert-path exists, and with this patch their setup would
> suddenly go from working (well, insecure but passing mail) to broken.
> They need to know where to look to find that documentation.
> 
> But it looks like this code path only triggers if you have set
> smtp-ssl-cert-path to something bogus. So anybody who does so at least
> knows about the option.
> 
> Which makes me wonder what happens when the cert path isn't defined by
> Git. The code says:
> 
> if (!defined $smtp_ssl_cert_path) {
> # use the OpenSSL defaults
> return (SSL_verify_mode => SSL_VERIFY_PEER());
> }
> 
> What does OpenSSL do when there is no cert? Hopefully it reports a
> verification failure (and in that sense your patch is making our code
> consistent with that, which is a good thing).

I suspect it's not very useful; I originally got here after setting up
git-send-email to talk to a server with a certificate signed by a
corporate CA and had to resort to the perl debugger to figure out where
it was going wrong.  There isn't even any output with --smtp-debug when
the SSL handshake fails.

The error message is (all on one line):

Unable to initialize SMTP properly. Check config and use --smtp-debug.
VALUES: server= encryption=ssl hello=
port=465 at /usr/libexec/git-core/git-send-email line 1357.

I wonder if we should do this to help debug SSL issues:

-- >8 --
diff --git a/git-send-email.perl b/git-send-email.perl
index e057051..6d4e0ee 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1317,6 +1317,10 @@ Message-Id: $message_id
require Net::SMTP::SSL;
$smtp_domain ||= maildomain();
require IO::Socket::SSL;
+   if ($debug_net_smtp) {
+   no warnings 'once';
+   $IO::Socket::SSL::DEBUG = 1;
+   }
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-- 8< --

> > Maybe we shouldn't worry too much about that, but should instead put the
> > invalid path into the error message:
> > 
> > die "CA path \"$smtp_ssl_cert_path\" does not exist.";
> 
> Given what I wrote above, yeah, I'd agree that is sufficient (and I do
> think mentioning the path is helpful).

I'll change it to this in a re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

2015-11-24 Thread John Keeping 
On Tue, Nov 24, 2015 at 05:28:21PM -0500, Jeff King wrote:
> On Tue, Nov 24, 2015 at 10:17:08PM +0000, John Keeping wrote:
> 
> > I wonder if we should do this to help debug SSL issues:
> > 
> > -- >8 --
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index e057051..6d4e0ee 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -1317,6 +1317,10 @@ Message-Id: $message_id
> > require Net::SMTP::SSL;
> > $smtp_domain ||= maildomain();
> > require IO::Socket::SSL;
> > +   if ($debug_net_smtp) {
> > +   no warnings 'once';
> > +   $IO::Socket::SSL::DEBUG = 1;
> > +   }
> > # Net::SMTP::SSL->new() does not forward any SSL options
> > IO::Socket::SSL::set_client_defaults(
> > ssl_verify_params());
> > -- 8< --
> 
> That certainly looks like a reasonable thing to be doing, assuming that
> the output from IO::Socket::SSL is generally helpful.

It's a bit verbose for errors, but it does let you know what went wrong:

DEBUG: .../IO/Socket/SSL.pm:1796: SSL connect attempt failed error:14090086:SSL 
routines:ssl3_get_server_certificate:certificate verify failed
DEBUG: .../IO/Socket/SSL.pm:673: fatal SSL error: SSL connect attempt failed 
error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify 
failed
DEBUG: .../IO/Socket/SSL.pm:1780: IO::Socket::IP configuration failed

It doesn't print anything when the SSL connection is established
successfully, but I don't think that's a problem and if we jump to
level 2 it starts logging things like:

DEBUG: .../IO/Socket/SSL.pm:687: waiting for fd to become ready: SSL wants a 
read first
DEBUG: .../IO/Socket/SSL.pm:707: socket ready, retrying connect
DEBUG: .../IO/Socket/SSL.pm:677: ssl handshake in progress

without adding anything useful.

> > > > Maybe we shouldn't worry too much about that, but should instead put the
> > > > invalid path into the error message:
> > > > 
> > > > die "CA path \"$smtp_ssl_cert_path\" does not exist.";
> > > 
> > > Given what I wrote above, yeah, I'd agree that is sufficient (and I do
> > > think mentioning the path is helpful).
> > 
> > I'll change it to this in a re-roll.
> 
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] send-email: die if CA path doesn't exist

2015-11-24 Thread John Keeping 
If the CA path isn't found it's most likely to indicate a
misconfiguration, in which case accepting any certificate is unlikely to
be the correct thing to do.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
Changes since v1:
- add missing path to error message
- remove trailing '.' on error message since die appends "at
  /path/to/git-send-email line ..."

 git-send-email.perl | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8e4c0e1..68c93fa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1196,8 +1196,7 @@ sub ssl_verify_params {
return (SSL_verify_mode => SSL_VERIFY_PEER(),
SSL_ca_file => $smtp_ssl_cert_path);
} else {
-   print STDERR "Not using SSL_VERIFY_PEER because the CA path 
does not exist.\n";
-   return (SSL_verify_mode => SSL_VERIFY_NONE());
+   die "CA path \"$smtp_ssl_cert_path\" does not exist";
}
 }
 
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] send-email: expand paths in sendemail.{to,cc}cmd config

2015-11-24 Thread John Keeping 
On Tue, Nov 24, 2015 at 05:23:30PM -0500, Jeff King wrote:
> On Tue, Nov 24, 2015 at 08:43:53AM +0000, John Keeping wrote:
> 
> > On Mon, Nov 23, 2015 at 07:04:46PM -0500, Eric Sunshine wrote:
> > > On Tue, Nov 17, 2015 at 5:01 PM, John Keeping <j...@keeping.me.uk> wrote:
> > > > These configuration variables specify the paths to commands so we should
> > > > support tilde-expansion for files inside a user's home directory.
> > > 
> > > Hmm, I don't see anything in the documentation which says that these
> > > are paths to commands, and the code itself treats them purely as
> > > commands to be invoked, not as paths to commands. What is the
> > > behavior, for instance, with --tocmd='foobar -x zopp' or even
> > > --tocmd='foobar -x ~/zopp'?
> > 
> > The path behaviour only expands leading '~' and '~user' (as documented
> > in git-config(1)):
> > 
> > $ git -c sendemail.tocmd='foobar -x ~/zopp' config --path 
> > sendemail.tocmd
> > foobar -x ~/zopp
> 
> We usually run user-supplied commands with a shell (and AFAICT, that is
> the case here). So wouldn't that turn into (when used by send-email):
> 
>   sh -c 'foobar -x ~/zopp'
> 
> and the shell would expand it for us? Running:
> 
>   git -c sendemail.tocmd='echo ~/foo' send-email -1
> 
> seems to work for me (it puts "/home/peff/foo" into the "to" header).

Ah, I hadn't tested it.  We can drop this patch then.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] send-email: enable SSL level 1 debug output

2015-11-24 Thread John Keeping 
If a server's certificate isn't accepted by send-email, the output is:

Unable to initialize SMTP properly. Check config and use --smtp-debug.

but adding --smtp-debug=1 just produces the same output since we don't
get as far as talking SMTP.

Turning on SSL debug at level 1 gives:

DEBUG: .../IO/Socket/SSL.pm:1796: SSL connect attempt failed 
error:14090086:SSL routines:ssl3_get_server_certificate:certificate verify 
failed
DEBUG: .../IO/Socket/SSL.pm:673: fatal SSL error: SSL connect attempt 
failed error:14090086:SSL routines:ssl3_get_server_certificate:certificate 
verify failed
DEBUG: .../IO/Socket/SSL.pm:1780: IO::Socket::IP configuration failed

IO::Socket::SSL defines level 1 debug as "print out errors from
IO::Socket::SSL and ciphers from Net::SSLeay".  In fact, it aliases
Net::SSLeay::trace which is defined to guarantee silence at level 0 and
only emit error messages at level 1, so let's enable it by default.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
This is the result of a previous discussion [0] but I decided to drop
the switch on --smtp-debug since level 1 only gives output on errors.

[0] http://marc.info/?l=git=144840344331208=2

 git-send-email.perl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index e907e0e..918aafa 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1318,6 +1318,7 @@ Message-Id: $message_id
require Net::SMTP::SSL;
$smtp_domain ||= maildomain();
require IO::Socket::SSL;
+   $IO::Socket::SSL::DEBUG = 1;
# Net::SMTP::SSL->new() does not forward any SSL options
IO::Socket::SSL::set_client_defaults(
ssl_verify_params());
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] send-email: expand paths in sendemail.{to,cc}cmd config

2015-11-24 Thread John Keeping 
On Mon, Nov 23, 2015 at 07:04:46PM -0500, Eric Sunshine wrote:
> On Tue, Nov 17, 2015 at 5:01 PM, John Keeping <j...@keeping.me.uk> wrote:
> > These configuration variables specify the paths to commands so we should
> > support tilde-expansion for files inside a user's home directory.
> 
> Hmm, I don't see anything in the documentation which says that these
> are paths to commands, and the code itself treats them purely as
> commands to be invoked, not as paths to commands. What is the
> behavior, for instance, with --tocmd='foobar -x zopp' or even
> --tocmd='foobar -x ~/zopp'?

The path behaviour only expands leading '~' and '~user' (as documented
in git-config(1)):

$ git -c sendemail.tocmd='foobar -x ~/zopp' config --path 
sendemail.tocmd
    foobar -x ~/zopp

> > Signed-off-by: John Keeping <j...@keeping.me.uk>
> > ---
> > diff --git a/git-send-email.perl b/git-send-email.perl
> > index 719c715..8e4c0e1 100755
> > --- a/git-send-email.perl
> > +++ b/git-send-email.perl
> > @@ -242,9 +242,7 @@ my %config_settings = (
> >  "smtpdomain" => \$smtp_domain,
> >  "smtpauth" => \$smtp_auth,
> >  "to" => \@initial_to,
> > -"tocmd" => \$to_cmd,
> >  "cc" => \@initial_cc,
> > -"cccmd" => \$cc_cmd,
> >  "aliasfiletype" => \$aliasfiletype,
> >  "bcc" => \@bcclist,
> >  "suppresscc" => \@suppress_cc,
> > @@ -259,6 +257,8 @@ my %config_settings = (
> >  my %config_path_settings = (
> >  "aliasesfile" => \@alias_files,
> >  "smtpsslcertpath" => \$smtp_ssl_cert_path,
> > +"tocmd" => \$to_cmd,
> > +"cccmd" => \$cc_cmd,
> >  );
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] send-email: die if CA path doesn't exist

2015-11-20 Thread John Keeping 
On Fri, Nov 20, 2015 at 06:18:48AM -0500, Jeff King wrote:
> On Tue, Nov 17, 2015 at 10:12:07PM +0000, John Keeping wrote:
> 
> > If the CA path isn't found it's most likely to indicate a
> > misconfiguration, in which case accepting any certificate is unlikely to
> > be the correct thing to do.
> 
> Yeah, this seems like a crazy default for security-sensitive code.
> 
> I suspect some people will see breakage from applying this (because
> their systems are broken and they did not know it), but that is a good
> thing.
> 
> For people who know their systems are broken and want to proceed anyway,
> what is the appropriate work-around? Obviously it involves disabling
> peer verification, but would we want to include instructions for doing
> so (either in the error message, or perhaps mentioning it in the commit
> message)?

The documentation already says:

Set it to an empty string to disable certificate verification.

It's a bit lost in the middle of a paragraph but I think that is the
best place for the detail of how to disable verification.

Having revisted the patch, I do think the message might be a bit terse,
but I can't think of a reasonably concise way to point at the
--smtp-ssl-cert-path argument as being the culprit.

Maybe we shouldn't worry too much about that, but should instead put the
invalid path into the error message:

die "CA path \"$smtp_ssl_cert_path\" does not exist.";
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] send-email: expand paths in sendemail.{to,cc}cmd config

2015-11-17 Thread John Keeping 
These configuration variables specify the paths to commands so we should
support tilde-expansion for files inside a user's home directory.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 git-send-email.perl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 719c715..8e4c0e1 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -242,9 +242,7 @@ my %config_settings = (
 "smtpdomain" => \$smtp_domain,
 "smtpauth" => \$smtp_auth,
 "to" => \@initial_to,
-"tocmd" => \$to_cmd,
 "cc" => \@initial_cc,
-"cccmd" => \$cc_cmd,
 "aliasfiletype" => \$aliasfiletype,
 "bcc" => \@bcclist,
 "suppresscc" => \@suppress_cc,
@@ -259,6 +257,8 @@ my %config_settings = (
 my %config_path_settings = (
 "aliasesfile" => \@alias_files,
 "smtpsslcertpath" => \$smtp_ssl_cert_path,
+"tocmd" => \$to_cmd,
+"cccmd" => \$cc_cmd,
 );
 
 # Handle Uncouth Termination
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] send-email: expand path in sendemail.smtpsslcertpath config

2015-11-17 Thread John Keeping 
As it says in the name, the SSL certificate path is a path so treat it
as one and support tilde-expansion.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 git-send-email.perl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index e907e0e..719c715 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -239,7 +239,6 @@ my %config_settings = (
 "smtpserveroption" => \@smtp_server_options,
 "smtpuser" => \$smtp_authuser,
 "smtppass" => \$smtp_authpass,
-"smtpsslcertpath" => \$smtp_ssl_cert_path,
 "smtpdomain" => \$smtp_domain,
 "smtpauth" => \$smtp_auth,
 "to" => \@initial_to,
@@ -259,6 +258,7 @@ my %config_settings = (
 
 my %config_path_settings = (
 "aliasesfile" => \@alias_files,
+"smtpsslcertpath" => \$smtp_ssl_cert_path,
 );
 
 # Handle Uncouth Termination
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC/PATCH] send-email: die if CA path doesn't exist

2015-11-17 Thread John Keeping 
If the CA path isn't found it's most likely to indicate a
misconfiguration, in which case accepting any certificate is unlikely to
be the correct thing to do.

Signed-off-by: John Keeping <j...@keeping.me.uk>
---
 git-send-email.perl | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 8e4c0e1..e057051 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1196,8 +1196,7 @@ sub ssl_verify_params {
return (SSL_verify_mode => SSL_VERIFY_PEER(),
SSL_ca_file => $smtp_ssl_cert_path);
} else {
-   print STDERR "Not using SSL_VERIFY_PEER because the CA path 
does not exist.\n";
-   return (SSL_verify_mode => SSL_VERIFY_NONE());
+   die "CA path does not exist.";
}
 }
 
-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] send-email config path expansion

2015-11-17 Thread John Keeping 
These two patches enable tilde-expansion for a few more config variables
in git-send-email.

The first case is the one that surprised me when it didn't work, the
second two are the other ones that look like they should be handled as
paths.

John Keeping (2):
  send-email: expand path in sendemail.smtpsslcertpath config
  send-email: expand paths in sendemail.{to,cc}cmd config

 git-send-email.perl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

-- 
2.6.3.462.gbe2c914

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git potential bug with fork-point

2015-11-07 Thread John Keeping 
On Mon, Nov 02, 2015 at 06:27:52AM +, Stenberg Jim (2) wrote:
> My problem:
> "Git merge-base --fork-point" acts unexpected when I refer to remote
> branches (typically "origin/".) With unexpected I mean that if I swap
> the position of the two references that the function takes as argument
> I get different results.  I highly suspect that this isn't a feature
> but a bug, or maybe I'm using the function in a way it wasn't intended
> to be used.
> I don't need you to fix it (swapping the arguments solves it), I just
> want you to be aware of it.
> 
> History & procedure:
> When  I was working on my automatic build script I came across the
> oddity that "Git merge-base --fork-point" behaved differently
> depending on the order in which the two references are passed.

I think this is expected.  The documentation for `--fork-point` says:

git merge-base --fork-point  []

Find the point at which a branch (or any history that leads to
) forked from another branch (or any reference) .
This does not just look for the common ancestor of the two
commits, but also takes into account the reflog of  to see
if the history leading to  forked from an earlier
incarnation of the branch  (see discussion on this mode
below).

Clearly the order of the arguments matters because the reflog is only
inspected for the `` argument.  Since the reflog is involved this
also means that the results are likely to be different between separate
copies of the same repository.

I suspect you do not want to be using `--fork-point` in your build
script; it is intended to help `git rebase` recover from history being
rewritten and if you do not need that behaviour you are probably better
off using the normal `git merge-base  ` mode, which
should give consistent results regardless of the order of the commits.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add git-grep threads param

2015-10-27 Thread John Keeping 
On Mon, Oct 26, 2015 at 10:25:41PM -0700, Victor Leschuk wrote:
> >> @@ -22,6 +22,7 @@ SYNOPSIS
> >>  [--color[=] | --no-color]
> >>  [--break] [--heading] [-p | --show-function]
> >>  [-A ] [-B ] [-C ]
> >> +[--threads ]
> 
> > Is this the best place for this option?  I know the current list isn't
> > sorted in any particular way, but here you're splitting up the set of
> > context options (`-A`, `-B`, `-C` and `-W`).
> 
> Agree, I'll move the option both here and in documentation.
> 
> >> -static int wait_all(void)
> >> +static int wait_all(struct grep_opt *opt)
> 
> > I'm not sure passing a grep_opt in here is the cleanest way to do this.
> > Options are a UI concept and all we care about here is the number of
> > threads.
> 
> > Since `threads` is a global, shouldn't the number of threads be a global
> > as well?  Could we reuse `use_threads` here (possibly renaming it
> > `num_threads`)?
> 
> This thought also crossed my mind, however we already pass grep_opt to
> start_threads() function, so I think passing it to wait_all() is not
> that ugly, and kind of symmetric. And I do not like the idea of
> duplicating same information in different places. What do you think?

The grep_opt in start_threads() is being passed through to run(), so it
seems slightly different to me.  If the threads were being setup in
grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in
grep_opt, but since this is local to this particular user of the grep
infrastructure adding num_threads to the grep_opt structure at all feels
wrong to me.

Note that I wasn't suggesting passing num_threads as a parameter to
wait_all(), but rather having it as global state that is accessed by
wait_all() in the same way as the `threads` array.

If we rename use_threads to num_threads and just use that, then we only
have the information in one place don't we?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add git-grep threads param

2015-10-27 Thread John Keeping 
On Tue, Oct 27, 2015 at 06:54:16AM -0700, Victor Leschuk wrote:
> Hello John,
> 
> >> This thought also crossed my mind, however we already pass grep_opt to
> >> start_threads() function, so I think passing it to wait_all() is not
> >> that ugly, and kind of symmetric. And I do not like the idea of
> >> duplicating same information in different places. What do you think?
> 
> > The grep_opt in start_threads() is being passed through to run(), so it
> > seems slightly different to me.  If the threads were being setup in
> > grep.c (as opposed to builtin/grep.c) then I'd agree that it belongs in
> > grep_opt, but since this is local to this particular user of the grep
> > infrastructure adding num_threads to the grep_opt structure at all feels
> > wrong to me.
> 
> > Note that I wasn't suggesting passing num_threads as a parameter to
> > wait_all(), but rather having it as global state that is accessed by
> > wait_all() in the same way as the `threads` array.
> 
> > If we rename use_threads to num_threads and just use that, then we only
> > have the information in one place don't we?
> 
> Yeah, I understood your idea. So we parse config_value directly to 
> 
> static int num_threads; /* old use_threads */

Presumably this is:

static int num_threads = -1;

so that the default behaviour continues to work correctly.

> And use it internally in builtin/grep.c. I think you are right.
> 
> Looks like grep_cmd_config() is the right place to parse it. Something like:
> 
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -267,6 +267,8 @@ static int wait_all(struct grep_opt *opt)
>  static int grep_cmd_config(const char *var, const char *value, void *cb)
>  {
> int st = grep_config(var, value, cb);
> +   if (thread_config(var, value, cb) < 0)
> +   st = -1;
> if (git_color_default_config(var, value, cb) < 0)
> st = -1;
> return st;
> 
> What do you think?

I'd be tempted to open code the "grep.threads" case in this function
rather than introducing a helper for a single variable, but I don't
think it matters either way.  This looks good.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] Add git-grep threads param

2015-10-26 Thread John Keeping 
On Mon, Oct 26, 2015 at 03:32:13PM +0300, Victor Leschuk wrote:
> Make number of git-grep worker threads a configuration parameter.
> According to several tests on systems with different number of CPU cores
> the hard-coded number of 8 threads is not optimal for all systems:
> tuning this parameter can significantly speed up grep performance.
> 
> Signed-off-by: Victor Leschuk 
> ---
>  Documentation/config.txt   |  4 
>  Documentation/git-grep.txt |  4 
>  builtin/grep.c | 34 
> ++
>  contrib/completion/git-completion.bash |  1 +
>  grep.c | 10 ++
>  grep.h |  2 ++
>  6 files changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..1c95587 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1447,6 +1447,10 @@ grep.extendedRegexp::
>   option is ignored when the 'grep.patternType' option is set to a value
>   other than 'default'.
>  
> +grep.threads::
> + Number of grep worker threads, use it to tune up performance on
> + multicore machines. Default value is 8.
> +
>  gpg.program::
>   Use this custom program instead of "gpg" found on $PATH when
>   making or verifying a PGP signature. The program must support the
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 4a44d6d..fbd4f83 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -22,6 +22,7 @@ SYNOPSIS
>  [--color[=] | --no-color]
>  [--break] [--heading] [-p | --show-function]
>  [-A ] [-B ] [-C ]
> +[--threads ]

Is this the best place for this option?  I know the current list isn't
sorted in any particular way, but here you're splitting up the set of
context options (`-A`, `-B`, `-C` and `-W`).

>  [-W | --function-context]
>  [-f ] [-e] 
>  [--and|--or|--not|(|)|-e ...]
> @@ -220,6 +221,9 @@ OPTIONS
>   Show  leading lines, and place a line containing
>   `--` between contiguous groups of matches.
>  
> +--threads ::
> + Set number of worker threads to . Default is 8.

The same comment as above applies here.

>  -W::
>  --function-context::
>   Show the surrounding text from the previous line containing a
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d04f440..5ef1b07 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -27,8 +27,7 @@ static char const * const grep_usage[] = {
>  static int use_threads = 1;
>  
>  #ifndef NO_PTHREADS
> -#define THREADS 8
> -static pthread_t threads[THREADS];
> +static pthread_t *threads;
>  
>  /* We use one producer thread and THREADS consumer
>   * threads. The producer adds struct work_items to 'todo' and the
> @@ -206,7 +205,8 @@ static void start_threads(struct grep_opt *opt)
>   strbuf_init([i].out, 0);
>   }
>  
> - for (i = 0; i < ARRAY_SIZE(threads); i++) {
> + threads = xcalloc(opt->num_threads, sizeof(pthread_t));
> + for (i = 0; i < opt->num_threads; i++) {
>   int err;
>   struct grep_opt *o = grep_opt_dup(opt);
>   o->output = strbuf_out;
> @@ -220,7 +220,7 @@ static void start_threads(struct grep_opt *opt)
>   }
>  }
>  
> -static int wait_all(void)
> +static int wait_all(struct grep_opt *opt)

I'm not sure passing a grep_opt in here is the cleanest way to do this.
Options are a UI concept and all we care about here is the number of
threads.

Since `threads` is a global, shouldn't the number of threads be a global
as well?  Could we reuse `use_threads` here (possibly renaming it
`num_threads`)?

>  {
>   int hit = 0;
>   int i;
> @@ -238,12 +238,14 @@ static int wait_all(void)
>   pthread_cond_broadcast(_add);
>   grep_unlock();
>  
> - for (i = 0; i < ARRAY_SIZE(threads); i++) {
> + for (i = 0; i < opt->num_threads; i++) {
>   void *h;
>   pthread_join(threads[i], );
>   hit |= (int) (intptr_t) h;
>   }
>  
> + free(threads);
> +
>   pthread_mutex_destroy(_mutex);
>   pthread_mutex_destroy(_read_mutex);
>   pthread_mutex_destroy(_attr_mutex);
> @@ -256,7 +258,7 @@ static int wait_all(void)
>  }
>  #else /* !NO_PTHREADS */
>  
> -static int wait_all(void)
> +static int wait_all(struct grep_opt *opt)
>  {
>   return 0;
>  }
> @@ -702,6 +704,8 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   N_("show  context lines before matches")),
>   OPT_INTEGER('A', "after-context", _context,
>   N_("show  context lines after matches")),
> + OPT_INTEGER(0, "threads", _threads,
> + N_("use  worker threads")),
>   OPT_NUMBER_CALLBACK(, N_("shortcut for -C NUM"),
>   context_callback),

Re: [PATCH] Add git-grep threads-num param

2015-10-22 Thread John Keeping 
On Thu, Oct 22, 2015 at 04:23:56PM +0300, Victor Leschuk wrote:
> Hello all, I suggest we make number of git-grep worker threads a configuration
> parameter. I have run several tests on systems with different number of CPU 
> cores.
> It appeared that the hard-coded number 8 lowers performance on both of my 
> systems:
> on my 4-core and 8-core systems the thread number of 4 worked about 20% 
> faster than
> default 8. So I think it is better to allow users tune this parameter.

For git-pack-objects we call the command-line parameter "--threads" and
the config variable "pack.threads".  Is there a reason not to use the
same name here (i.e. "--threads" and "grep.threads")?

> Signed-off-by: Victor Leschuk 
> ---
>  Documentation/config.txt   |  4 
>  Documentation/git-grep.txt |  5 +
>  builtin/grep.c | 20 +---
>  contrib/completion/git-completion.bash |  1 +
>  grep.c | 15 +++
>  grep.h |  4 
>  6 files changed, 42 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..c3df20c 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1447,6 +1447,10 @@ grep.extendedRegexp::
>   option is ignored when the 'grep.patternType' option is set to a value
>   other than 'default'.
>  
> +grep.threadsNum::
> + Number of grep worker threads, use it to tune up performance on
> + multicore machines. Default value is 8.
> +
>  gpg.program::
>   Use this custom program instead of "gpg" found on $PATH when
>   making or verifying a PGP signature. The program must support the
> diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
> index 4a44d6d..e9ca265 100644
> --- a/Documentation/git-grep.txt
> +++ b/Documentation/git-grep.txt
> @@ -22,6 +22,7 @@ SYNOPSIS
>  [--color[=] | --no-color]
>  [--break] [--heading] [-p | --show-function]
>  [-A ] [-B ] [-C ]
> +[-t ]
>  [-W | --function-context]
>  [-f ] [-e] 
>  [--and|--or|--not|(|)|-e ...]
> @@ -220,6 +221,10 @@ OPTIONS
>   Show  leading lines, and place a line containing
>   `--` between contiguous groups of matches.
>  
> +-t ::
> +--threads-num ::
> + Set number of worker threads to . Default is 8.
> +
>  -W::
>  --function-context::
>   Show the surrounding text from the previous line containing a
> diff --git a/builtin/grep.c b/builtin/grep.c
> index d04f440..9b4fc47 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -27,8 +27,7 @@ static char const * const grep_usage[] = {
>  static int use_threads = 1;
>  
>  #ifndef NO_PTHREADS
> -#define THREADS 8
> -static pthread_t threads[THREADS];
> +static pthread_t *threads;
>  
>  /* We use one producer thread and THREADS consumer
>   * threads. The producer adds struct work_items to 'todo' and the
> @@ -206,7 +205,8 @@ static void start_threads(struct grep_opt *opt)
>   strbuf_init([i].out, 0);
>   }
>  
> - for (i = 0; i < ARRAY_SIZE(threads); i++) {
> + threads = xmalloc(sizeof(pthread_t) * opt->num_threads);
> + for (i = 0; i < opt->num_threads; i++) {
>   int err;
>   struct grep_opt *o = grep_opt_dup(opt);
>   o->output = strbuf_out;
> @@ -220,7 +220,7 @@ static void start_threads(struct grep_opt *opt)
>   }
>  }
>  
> -static int wait_all(void)
> +static int wait_all(struct grep_opt *opt)
>  {
>   int hit = 0;
>   int i;
> @@ -238,12 +238,14 @@ static int wait_all(void)
>   pthread_cond_broadcast(_add);
>   grep_unlock();
>  
> - for (i = 0; i < ARRAY_SIZE(threads); i++) {
> + for (i = 0; i < opt->num_threads; i++) {
>   void *h;
>   pthread_join(threads[i], );
>   hit |= (int) (intptr_t) h;
>   }
>  
> + free(threads);
> +
>   pthread_mutex_destroy(_mutex);
>   pthread_mutex_destroy(_read_mutex);
>   pthread_mutex_destroy(_attr_mutex);
> @@ -256,7 +258,7 @@ static int wait_all(void)
>  }
>  #else /* !NO_PTHREADS */
>  
> -static int wait_all(void)
> +static int wait_all(struct grep_opt *opt)
>  {
>   return 0;
>  }
> @@ -702,6 +704,10 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>   N_("show  context lines before matches")),
>   OPT_INTEGER('A', "after-context", _context,
>   N_("show  context lines after matches")),
> +#ifndef NO_PTHREADS
> + OPT_INTEGER('t', "threads-num", _threads,
> + N_("use  worker threads")),
> +#endif /* !NO_PTHREADS */
>   OPT_NUMBER_CALLBACK(, N_("shortcut for -C NUM"),
>   context_callback),
>   OPT_BOOL('p', "show-function", ,
> @@ -910,7 +916,7 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>

Re: [BUG] Broken links in Git Documentation/user-manual.txt

2015-10-14 Thread John Keeping 
On Wed, Oct 14, 2015 at 09:37:05AM +0200, Matthieu Moy wrote:
> Xue Fuqiao  writes:
> 
> > Hi list,
> >
> > In https://git-scm.com/docs/user-manual.html , all links to the
> > glossary[1] are broken.
> 
> Actually, the links themselves are fine, but the destimation is broken.
> 
> The doc is supposed to look like this :
> 
>   https://www.kernel.org/pub/software/scm/git/docs/user-manual.html#def_head
> 
> with the glossary at the end. On
> https://git-scm.com/docs/user-manual.html, the glossary is displayed as
> verbatim text.
> 
> This does not seem to be a bug in our user-manual.txt, but in the way
> it's processed by git-scm.com.

I think it was an issue in the source, but was fixed by be510e0
(Documentation: fix section header mark-up, 2015-09-25).  I'm not sure
when/how git-scm.com rebulds its documentation, but I'm pretty sure that
fix hasn't made it into a release yet so I doubt the site has picked it
up.

> I reported the issue there:
> 
> https://github.com/git/git-scm.com/issues/605
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] parallel make interdepencies

2015-10-06 Thread John Keeping 
On Tue, Oct 06, 2015 at 03:13:05PM +0200, Johannes Schindelin wrote:
> Hi Michael,
> 
> On 2015-10-06 10:12, Michael J Gruber wrote:
> > "make -j3" just errored out on me, a follow-up "make" succeeded". This
> > looks like an interdependency issue, but I don't know how to track it:
> > 
> > GEN git-web--browse
> > GEN git-add--interactive
> > GEN git-difftool
> > mv: der Aufruf von stat für „perl.mak“ ist nicht möglich: Datei oder
> > Verzeichnis nicht gefunden
> > 
> > (cannot stat "perl.mak")
> 
> This one sounds awfully familiar. Although I only encountered this if
> I specified `make -j15 clean all`, i.e. *both* "clean" and "all"...

I've seen something like this after upgrading perl (I can't remember the
exact error, so it may not be the same problem but I'm pretty sure it
involves perl.mak).  The problem was a result of the perl library path
changing, but I never got around to creating a patch.

I thought I remembered someone else posting a patch to address this, but
I can't find it so perhaps I'm remembering commit 07981dc (Makefile:
rebuild perl scripts when perl paths change, 2013-11-18).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How can I ignore insignificant change during merge ?

2015-10-05 Thread John Keeping 
On Mon, Oct 05, 2015 at 10:13:00PM +0200, Jens Brejner wrote:
> I need to merge a branch, +100k changes. The vast majority of changes
> are insignificant, because they only represent a screen position in
> the editor, so these changes should never have been in git - but but
> MadCap Flare already put them there.
> 
> The files in question are xml, and the difference can be exemplifed like this:
> 
> Original (when branches were created):
> html xmlns:MadCap="http://www.madcapsoftware.com/Schemas/MadCap.xsd;
> MadCap:lastBlockDepth="5" MadCap:lastHeight="32"
> MadCap:lastWidth="400"
> Branch1:
> html xmlns:MadCap="http://www.madcapsoftware.com/Schemas/MadCap.xsd;
> MadCap:lastBlockDepth="5" MadCap:lastHeight="24"
> MadCap:lastWidth="500"
> Branch2:
> html xmlns:MadCap="http://www.madcapsoftware.com/Schemas/MadCap.xsd;
> MadCap:lastBlockDepth="5" MadCap:lastHeight="41"
> MadCap:lastWidth="300"
> 
> How can git help me so files where the only difference matches
> something like this regex:
> /html xmlns:.* MadCap:lastHeight="\d+" MadCap:lastWidth="\d+"/
> 
> for the files that qualify, I want git to ignore the change, and
> therefore the merge-conflict, and then just accept "my" file for the
> merged changeset.
> 
> Any suggestions on how to I can have git help me with that ?

Have you looked into defining a custom merge driver for these files?
It's documented in the "Performing a three-way merge" section of
gitattributes(5) - that is, "git help attributes".

It should be relatively easy to ignore these changes, but you'll have to
deal with merging the rest of the files as well; Python's difflib module
may be useful.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  1   2   3   4   5   6   7   8   9   >