Re: [PATCH v2] completion: Add '--edit-todo' to rebase

[John Keeping]

On Thu, Jul 30, 2015 at 01:24:03PM +0200, SZEDER Gábor wrote:
> 
> Quoting Thomas Braun :
> 
> > Signed-off-by: Thomas Braun 
> > ---
> >> John Keeping  hat am 13. Juli 2015 um 15:11 
> >> geschrieben:
> >> git-rebase.sh contains:
> >>
> >>if test "$action" = "edit-todo" && test "$type" != "interactive"
> >>then
> >>die "$(gettext "The --edit-todo action can only be used during 
> >> interactive
> >> rebase.")"
> >>fi
> >>
> >> I wonder if it's worth doing a similar check here, which presumably
> >> means testing if "$dir"/interactive exists.
> >
> > Good point. Thanks for the hint.
> 
> Perhaps the subject line could say "completion: offer '--edit-todo'  
> during interactive rebase" to be a bit more specific.
> 
> > contrib/completion/git-completion.bash | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash
> > b/contrib/completion/git-completion.bash
> > index c97c648..b03050e 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -1668,7 +1668,11 @@ _git_rebase ()
> > {
> > local dir="$(__gitdir)"
> > if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
> > -   __gitcomp "--continue --skip --abort"
> > +   if [ -d "$dir"/interactive ]; then
> 
> This doesn't work for me, I think it looks for the right file at the  
> wrong place.  During an interactive rebase I have no  
> '.git/interactive' file but a '.git/rebase-merge/interactive', so I  
> never get '--edit-todo'.
> 
> After some playing around and a cursory look at the source it seems to  
> me that I have '.git/rebase-apply' during a "regular" rebase and  
> '.git/rebase-merge' during an interactive rebase, and git-rebase.sh  
> checks the presence of the 'interactive' file only in  
> '.git/rebase-merge'.  It's not clear to me yet whether it's possible  
> to have a '.git/rebase-merge' without the file 'interactive' in it.   
> If it is possible, then I'd like to know with which commands and under  
> what circumstances.  If it isn't, then we wouldn't have to look for  
> the file at all, because checking the presence of the directory would  
> be enough.

"git rebase --merge" will use ".git/rebase-merge" without creating the
"interactive" flag.
--
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: Merging after directory got turned into submodule

[John Keeping]

On Tue, Aug 11, 2015 at 03:02:41PM +0200, Martin von Gagern wrote:
> I've noticed that if I turn a subdirectory into a submodule, I'm having
> severe trouble merging branches from before that change, even if they
> don't modify that subdirectory at all.
> 
> I've posted this problem on Stack Overflow and started a bounty for it.
> See http://stackoverflow.com/q/31821219/1468366. So far I haven't
> received an answer, so I decided to ask here as well.
> 
> Here is an example.
> 
> # Create one project, to be used as a subproject later on
> git init a
> cd a
> echo aaa > aa
> git add -A
> git commit -m a1
> cd ..
> 
> # Create a second project, containing a as a normal directory initially
> git init b
> cd b
> mkdir a b
> echo aaa > a/aa
> echo bbb > b/bb
> git add -A
> git commit -m b1
> 
> # Replace directory with submodule
> git rm -r a
> git submodule add ../a a
> git commit -m b2
> 
> # Create feature brach starting at version without submodule
> git submodule deinit .  # will error if I don't do this
> git checkout -b branch HEAD^
> echo abc > b/bb
> git commit -a -m b3
> 
> # Try to merge the feature branch
> git checkout master
> git merge branch
> 
> This prints an error message:
> 
> > CONFLICT (file/directory): There is a directory with name a in branch.
> > Adding a as a~HEAD
> > Automatic merge failed; fix conflicts and then commit the result.
> 
> I get the same error if I do a git submodule update --init before the
> git merge branch. I don't see any a~HEAD anywhere, neither in my
> directory tree nor in the output from git status, which reads like this:
> 
> > On branch master
> > You have unmerged paths.
> >   (fix conflicts and run "git commit")
> >
> > Changes to be committed:
> >
> > modified:   b/bb
> >
> > Unmerged paths:
> >   (use "git add ..." to mark resolution)
> >
> > added by us: a
> 
> If I do git add a as suggested, I get another error:

"git reset a" works for me (i.e. set a to what it was before merging).
If you then commit and check "git ls-tree HEAD" it shows "a" is still a
submodule.
--
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-remote-helper behavior on Windows, not recognizing blank line as terminator

[John Keeping]

On Sun, Aug 23, 2015 at 11:40:17AM -0700, Anish Athalye wrote:
> I'm having some issues with git remote helper behavior on Windows.
> 
> According to the protocol
> (https://www.kernel.org/pub/software/scm/git/docs/gitremote-helpers.html),
> when doing things like listing capabilities, git expects the remote
> helper to send back a blank line when it's done.
> 
> I'm having trouble having git recognize the blank line (see
> https://github.com/anishathalye/git-remote-dropbox/issues/13#issuecomment-133894730
> for details).
> 
> Has anyone come across this behavior before? Am I doing something
> wrong, or could there be a bug in git? What's the best way to proceed?
> 
> 
> Any help or suggestions would be greatly appreciated!

The remote-helper parser tends to be very strict about its input.  I
suspect that on Windows you are sending CRLF rather than LF, so Git sees
a line containing CR.

By default the stdio streams are probably open in "text" mode, which
will convert "\n" to "\r\n".  You probably need to reopen stdout in
binary mode to make sure the output is correct.
--
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: index file list files not found in working tree

[John Keeping]

On Tue, Aug 25, 2015 at 12:16:43PM +0200, Rafik E Younan wrote:
> I got a recommendation to use reset --hard. I tried it and it says the 
> HEAD is now at correct commit, but missing files are not restored!
> 
> I tried `ls-tree --name-only` and it lists missing files and folders, 
> but the actual working tree doesn't have these files and folders.

Is there any chance that you have enabled sparse checkouts?  See the
documentation at the bottom of git-read-tree(1).

> On 08/24/2015 12:34 PM, Rafik E Younan wrote:
> > Hi,
> >
> > After several merges and rebases I finally got my branches and history 
> > to reflect valid commits and proper history. Everything is pushed to 
> > internal bare repo and the remotes seems OK.
> >
> > When I clone the updated repository, all branches reflect the correct 
> > updated trees and blobs.
> >
> > The problem occurs only on the original local repository where all the 
> > merging and re-basing took place!
> >
> > When I checkout a branch, several files and folders are deleted from 
> > the working tree. When I examine the history of these files, there are 
> > only commits of adding them and modifying them but no log for deleting 
> > them, and they aren't deleted when I checkout the same branch in 
> > another fresh cloned repo.
> >
> > Git status command doesn't indicate any changes in these files. I 
> > found the files and folders names in the `.git/index` file. So after 
> > manually removing the `.git/index` file and usinge `git reset` 
> > command, `git status` indicates that the files and folders are deleted.
> >
> > I use `git checkout -- ...` and restore all 
> > missing files and folders, just then the working tree matches the 
> > fresh checkout of the same branch on any other cloned repo.
> >
> > After examining the tree object of the current commit, all files and 
> > folders exists, although clearly the checkout missed some of them!
> >
> > Because the repository is local and private, I can't share any url for 
> > publicly accessible repository, and if one exists, no problem could be 
> > found, because the problem resides in just this certain local clone.
> >
> > Answering the following questions might give some clues for the problem:
> > * How does git populate the index file after every branch checkout?
> > * Is there any object to reflect the content of the index file?
> >
> > I would appreciate any pointers for where the problem could be.
> >
> > Thanks,
> > Rafik
> >
> 
> --
> 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

[RFC/PATCH] date: allow any format to display local time

[John Keeping]

Make DATE_LOCAL a bit flag that may be combined with the other formats.
In order to keep date_mode_type as a true enumeration the possible
combinations are included explicitly (except "relative local time" which
is nonsensical).

Signed-off-by: John Keeping 
---
I primarily want this to make life easier in CGit (so that we can reuse
libgit.a for formatting dates in the originator's timezone), but I think
it makes sense to expose these options to the user in general.

 builtin/blame.c |  3 +--
 cache.h |  9 +++--
 date.c  | 31 ---
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..dff6934 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2575,7 +2575,7 @@ parse_done:
}
 
/* The maximum width used to show the dates */
-   switch (blame_date_mode.type) {
+   switch (blame_date_mode.type & ~DATE_LOCAL) {
case DATE_RFC2822:
blame_date_width = sizeof("Thu, 19 Oct 2006 16:00:04 -0700");
break;
@@ -2600,7 +2600,6 @@ parse_done:
   fewer display columns. */
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 
1; /* add the null */
break;
-   case DATE_LOCAL:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index 4e25271..cda5c51 100644
--- a/cache.h
+++ b/cache.h
@@ -1091,12 +1091,17 @@ struct date_mode {
DATE_NORMAL = 0,
DATE_RELATIVE,
DATE_SHORT,
-   DATE_LOCAL,
DATE_ISO8601,
DATE_ISO8601_STRICT,
DATE_RFC2822,
DATE_STRFTIME,
-   DATE_RAW
+   DATE_RAW,
+   DATE_LOCAL = 0x80,
+   DATE_SHORT_LOCAL = (DATE_SHORT | DATE_LOCAL),
+   DATE_ISO8601_LOCAL = (DATE_ISO8601 | DATE_LOCAL),
+   DATE_ISO8601_STRICT_LOCAL = (DATE_ISO8601_STRICT | DATE_LOCAL),
+   DATE_RFC2822_LOCAL = (DATE_RFC2822 | DATE_LOCAL),
+   DATE_STRFTIME_LOCAL = (DATE_STRFTIME | DATE_LOCAL),
} type;
const char *strftime_fmt;
 };
diff --git a/date.c b/date.c
index 8f91569..e0e0f3b 100644
--- a/date.c
+++ b/date.c
@@ -163,7 +163,7 @@ void show_date_relative(unsigned long time, int tz,
 struct date_mode *date_mode_from_type(enum date_mode_type type)
 {
static struct date_mode mode;
-   if (type == DATE_STRFTIME)
+   if (type == DATE_STRFTIME || type == DATE_STRFTIME_LOCAL)
die("BUG: cannot create anonymous strftime date_mode struct");
mode.type = type;
return &mode;
@@ -173,6 +173,7 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
 {
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
+   enum date_mode_type type = mode->type;
 
if (mode->type == DATE_RAW) {
strbuf_reset(&timebuf);
@@ -189,8 +190,10 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
return timebuf.buf;
}
 
-   if (mode->type == DATE_LOCAL)
+   if (type & DATE_LOCAL) {
tz = local_tzoffset(time);
+   type &= ~DATE_LOCAL;
+   }
 
tm = time_to_tm(time, tz);
if (!tm) {
@@ -199,17 +202,17 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
}
 
strbuf_reset(&timebuf);
-   if (mode->type == DATE_SHORT)
+   if (type == DATE_SHORT)
strbuf_addf(&timebuf, "%04d-%02d-%02d", tm->tm_year + 1900,
tm->tm_mon + 1, tm->tm_mday);
-   else if (mode->type == DATE_ISO8601)
+   else if (type == DATE_ISO8601)
strbuf_addf(&timebuf, "%04d-%02d-%02d %02d:%02d:%02d %+05d",
tm->tm_year + 1900,
tm->tm_mon + 1,
tm->tm_mday,
tm->tm_hour, tm->tm_min, tm->tm_sec,
tz);
-   else if (mode->type == DATE_ISO8601_STRICT) {
+   else if (type == DATE_ISO8601_STRICT) {
char sign = (tz >= 0) ? '+' : '-';
tz = abs(tz);
strbuf_addf(&timebuf, 
"%04d-%02d-%02dT%02d:%02d:%02d%c%02d:%02d",
@@ -218,12 +221,12 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
tm->tm_mday,
tm->tm_hour, tm->tm_min, tm->tm_sec,
sign, tz / 100, tz % 100);
-   } else if (m

Re: [RFC/PATCH] date: allow any format to display local time

[John Keeping]

On Mon, Aug 31, 2015 at 02:50:18PM -0400, Jeff King wrote:
> On Mon, Aug 31, 2015 at 10:28:24AM -0700, Junio C Hamano wrote:
> > I am unhappy with the change to blame.c, and that is not because I
> > do not want "blame" to be touched.
> > 
> > The fact that this change has to touch it indicates that it is easy
> > for other new callers of date formatting code to forget masking
> > DATE_LOCAL bit like this patch does when they want to change their
> > behaviour based on the settings of date-mode.  And it would be hard
> > to catch such a subtle breakage during future reviews.
> 
> Yeah, my first instinct on seeing the bitfield was that it would
> probably be much simpler to keep the enum pure, and add a bit to the
> struct. A patch in that direction is below. I think the result is that
> the using code is much cleaner, and the complexity of converting
> "foo-local" into the enum/bitfield combination is contained in
> parse_date_format.

Yes, the result below is much cleaner than my version.

> > I agree that among "enum date_mode_type", DATE_LOCAL is an oddball.
> > It should be able to act as an orthogonal and independent flag with
> > at least some, like NORMAL, SHORT, etc.  Specifying DATE_LOCAL with
> > some others at the same time, however, would not make much sense,
> > would it?  What does it even mean to say time as relative under
> > DATE_LOCAL bit?
> 
> I think the "relative local" thing is not _too_ bad, as John's patch
> enforces it at the user-level (it does not parse "relative-local" at
> all, and mine similarly complains).
> 
> > >   else
> > >   strbuf_addf(&timebuf, "%.3s %.3s %d %02d:%02d:%02d %d%c%+05d",
> > 
> > What cannot be seen in the post-context of this hunk is that we
> > deliberately drop the timezone information when tweaking the
> > "normal" format to "local".  This is done only in the final else
> > clause in show_date() because the code knows that LOCAL is not an
> > independent bit.
> 
> I didn't address this in my patch (except to convert the check of
> DATE_LOCAL to mode->local). I agree that we should handle it in other
> formats, too, but I think it must be format-dependent. Certainly "rfc"
> and "iso" must always show the timezone. I'd argue that "raw" probably
> should, as well. That leaves only "short" and "relative", which do not
> display the time zone in the first place. So all of the formats are
> covered, I think.
> 
> > I think the original motivation is that there is no need to show the
> > timezone information when the user knows the time is shown in the
> > local timezone---after all, the --date=local option was given by her
> > in the first place.  This kind of tweaking should be made consistent
> > with other variants, now the other variants are interacting with
> > this LOCAL bit.  If we were to go forward with this patch, I think
> > we probably should remove this special casing of "showing normal
> > date in localzone, drop the zone information", as we cannot sanely
> > drop the TZ offset from the output of some of the formats and stay
> > valid (e.g RFC2822).
> 
> I think I'd rather remain inconsistent between the formats (which, after
> all, do not need to show exactly the same information), then have people
> complain that "--date=local" is regressed and now shows a timezone.
> 
> > > @@ -777,14 +780,25 @@ void parse_date_format(const char *format, struct 
> > > date_mode *mode)
> > >   else if (!strcmp(format, "iso8601") ||
> > >!strcmp(format, "iso"))
> > >   mode->type = DATE_ISO8601;
> > > + else if (!strcmp(format, "iso8601-local") ||
> > > +  !strcmp(format, "iso-local"))
> > > + mode->type = DATE_ISO8601_LOCAL;
> 
> I found the manual "-local" handling here to be a little, well...manual.
> So in the patch below I've revamped the parsing to look left-to-right
> for each component: type, local modifier, strftime format.
> 
> It ends up being about the same amount of code, but has two advantages:
> 
>   1. It's more flexible if we want to make more modifiers later. In
>  fact, it would be trivial to implement the current "-strict" as a
>  separate flag if we chose. I don't think there is much point in
>  doing so, but we could do something like "default-local-strict"
>  for showing the local time _with_ the timezone.
> 
>   2. We can provide more specific error messages (like "relative does
>  not make sense with -local", rather than "unknown date format:
>  relative-local").
> 
> But that is somewhat orthogonal to the enum thing. We could leave the
> parsing as John has it, and just set mode->local as appropriate in each
> conditional block.

I like that the parsing indicates that the format and "local-ness" are
orthogonal in this version.

Are you willing to resend this as a proper patch?

I'm happy to build documentation & tests on top, although there don't
seem to be any tests for date formats outside t6300-for-each-ref.sh at
the moment (and the "format" mode is neither tested

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Mon, Aug 31, 2015 at 04:48:32PM -0400, Jeff King wrote:
> Most of our "--date" modes are about the format of the date:
> which items we show and in what order. But "--date=local" is
> a bit of an oddball. It means "show the date in the normal
> format, but using the local timezone". The timezone we use
> is orthogonal to the actual format, and there is no reason
> we could not have "localized iso8601", etc.
> 
> This patch adds a "local" boolean field to "struct
> date_mode", and drops the DATE_LOCAL element from the
> date_mode_type enum (it's now just DATE_NORMAL plus
> local=1). The new feature is accessible to users by adding
> "-local" to any date mode (e.g., "iso-local"), and we retain
> "local" as an alias for "default-local" for backwards
> compatibility.
> 
> Signed-off-by: Jeff King 
> ---

This fails t6300 with:

fatal: unknown date-mode modifier: my date is %Y-%m-%d
not ok 83 - Check format of strftime date fields
#
#   echo "my date is 2006-07-03" >expected &&
#   git for-each-ref \
# --format="%(authordate:format:my date is %Y-%m-%d)" \
# refs/heads >actual &&
#   test_cmp expected actual
#

The following squash-in fixes it:

diff --git a/date.c b/date.c
index aa57cad..3aa8002 100644
--- a/date.c
+++ b/date.c
@@ -817,9 +817,7 @@ void parse_date_format(const char *format, struct date_mode 
*mode)
if (!skip_prefix(p, ":", &p))
die("date format missing colon separator: %s", format);
mode->strftime_fmt = xstrdup(p);
-   }
-
-   if (*p)
+   } else if (*p)
die("unknown date-mode modifier: %s", p);
 }
 

> I wonder if anybody would be confused and try "iso-local-strict", which
> does not work with this code. If we bumped "-strict" to become a
> modifier (like "-local"), we could easily make the order
> interchangeable.
> 
>  Documentation/rev-list-options.txt | 21 ---
>  builtin/blame.c|  1 -
>  cache.h|  2 +-
>  date.c | 77 
> +-
>  4 files changed, 67 insertions(+), 34 deletions(-)
> 
> diff --git a/Documentation/rev-list-options.txt 
> b/Documentation/rev-list-options.txt
> index a9b808f..35dc44b 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -702,12 +702,16 @@ include::pretty-options.txt[]
>  --date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
>   Only takes effect for dates shown in human-readable format, such
>   as when using `--pretty`. `log.date` config variable sets a default
> - value for the log command's `--date` option.
> + value for the log command's `--date` option. By default, dates
> + are shown in the original time zone (either committer's or
> + author's). If `-local` is appended to the format (e.g.,
> + `iso-local`), the user's local time zone is used instead.
>  +
>  `--date=relative` shows dates relative to the current time,
> -e.g. ``2 hours ago''.
> +e.g. ``2 hours ago''. The `-local` option cannot be used with
> +`--relative`.
>  +
> -`--date=local` shows timestamps in user's local time zone.
> +`--date=local` is an alias for `--date=default-local`.
>  +
>  `--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like 
> format.
>  The differences to the strict ISO 8601 format are:
> @@ -730,10 +734,15 @@ format, often found in email messages.
>  `--date=format:...` feeds the format `...` to your system `strftime`.
>  Use `--date=format:%c` to show the date in your system locale's
>  preferred format.  See the `strftime` manual for a complete list of
> -format placeholders.
> +format placeholders. When using `-local`, the correct syntax is
> +`--date=format-local:...`.
>  +
> -`--date=default` shows timestamps in the original time zone
> -(either committer's or author's).
> +`--date=default` is the default format, and is similar to
> +`--date=rfc2822`, with a few exceptions:
> +
> + - there is no comma after the day-of-week
> +
> + - the time zone is omitted when the local time zone is used
>  
>  ifdef::git-rev-list[]
>  --header::
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4db01c1..6fd1a63 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2600,7 +2600,6 @@ parse_done:
>  fewer display columns. */
>   blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 
> 1; /* add the null */
>   break;
> - case DATE_LOCAL:
>   case DATE_NORMAL:
>   blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
>   break;
> diff --git a/cache.h b/cache.h
> index 4e25271..9a91b1d 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1091,7 +1091,6 @@ struct date_mode {
>   DATE_NORMAL = 0,
>   DATE_RELATIVE,
>   DATE_SHORT,
> - DATE_LOCAL,
>   DATE_ISO8601,
> 

Re: [PATCH 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Mon, Aug 31, 2015 at 06:05:09PM -0400, Jeff King wrote:
> On Mon, Aug 31, 2015 at 05:33:37PM -0400, Jeff King wrote:
> 
> > > diff --git a/date.c b/date.c
> > > index aa57cad..3aa8002 100644
> > > --- a/date.c
> > > +++ b/date.c
> > > @@ -817,9 +817,7 @@ void parse_date_format(const char *format, struct 
> > > date_mode *mode)
> > >   if (!skip_prefix(p, ":", &p))
> > >   die("date format missing colon separator: %s", format);
> > >   mode->strftime_fmt = xstrdup(p);
> > > - }
> > > -
> > > - if (*p)
> > > + } else if (*p)
> > >   die("unknown date-mode modifier: %s", p);
> > 
> > Yeah, that works. We could also advance "p" in the DATE_STRFTIME
> > conditional, but I think your solution is less ugly.
> > 
> > Thanks for debugging my mess.
> 
> By the way, I was imagining you would pick these up and add to them with
> more tests and documentation. If that's the case, please feel free to
> squash that in and keep my signoff. If not, then I can post a re-roll
> after waiting for other comments.

OK, I'll send them with some additions to t6300 built on top, although
it may take a couple of days.

The other documentation improvements feel like an independent topic that
isn't necessary for this series to graduate; I'd prefer not to block
this waiting for those changes.
--
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/6] Make "local" orthogonal to date format

[John Keeping]

Jeff's first patch is unmodified but I've squashed the fixed currently
on "pu" into the second.  I also realised while adding the tests that
"raw-local" is meaningless so I've modified the code to reject it in the
same way as "relative-local".

Jeff King (2):
  fast-import: switch crash-report date to iso8601
  date: make "local" orthogonal to date format

John Keeping (4):
  t6300: introduce test_date() helper
  t6300: make UTC and local dates different
  t6300: add test for "raw" date format
  t6300: add tests for "-local" date formats

 Documentation/rev-list-options.txt |  21 --
 builtin/blame.c|   1 -
 cache.h|   2 +-
 date.c |  77 +---
 fast-import.c  |   2 +-
 t/t6300-for-each-ref.sh| 139 +++--
 6 files changed, 140 insertions(+), 102 deletions(-)

-- 
2.5.0.466.g9af26fa

--
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/6] fast-import: switch crash-report date to iso8601

[John Keeping]

From: Jeff King 

When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.

This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.

Signed-off-by: Jeff King 
Signed-off-by: John Keeping 
---
 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,7 @@ static void write_crash_report(const char *err)
fprintf(rpt, "fast-import crash report:\n");
fprintf(rpt, "fast-import process: %"PRIuMAX"\n", (uintmax_t) 
getpid());
fprintf(rpt, "parent process : %"PRIuMAX"\n", (uintmax_t) 
getppid());
-   fprintf(rpt, "at %s\n", show_date(time(NULL), 0, DATE_MODE(LOCAL)));
+   fprintf(rpt, "at %s\n", show_date(time(NULL), 0, 
DATE_MODE(ISO8601)));
fputc('\n', rpt);
 
fputs("fatal: ", rpt);
-- 
2.5.0.466.g9af26fa

--
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/6] date: make "local" orthogonal to date format

[John Keeping]

From: Jeff King 

Most of our "--date" modes are about the format of the date:
which items we show and in what order. But "--date=local" is
a bit of an oddball. It means "show the date in the normal
format, but using the local timezone". The timezone we use
is orthogonal to the actual format, and there is no reason
we could not have "localized iso8601", etc.

This patch adds a "local" boolean field to "struct
date_mode", and drops the DATE_LOCAL element from the
date_mode_type enum (it's now just DATE_NORMAL plus
local=1). The new feature is accessible to users by adding
"-local" to any date mode (e.g., "iso-local"), and we retain
"local" as an alias for "default-local" for backwards
compatibility.

Signed-off-by: Jeff King 
Signed-off-by: John Keeping 
---
This is Jeff's original patch with my fixup for DATE_STRFTIME squashed
in and a new change to reject "raw-local" (in both Documentation/ and
date.c).

 Documentation/rev-list-options.txt | 21 ---
 builtin/blame.c|  1 -
 cache.h|  2 +-
 date.c | 77 +-
 4 files changed, 67 insertions(+), 34 deletions(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index a9b808f..5d28133 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
 --date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
Only takes effect for dates shown in human-readable format, such
as when using `--pretty`. `log.date` config variable sets a default
-   value for the log command's `--date` option.
+   value for the log command's `--date` option. By default, dates
+   are shown in the original time zone (either committer's or
+   author's). If `-local` is appended to the format (e.g.,
+   `iso-local`), the user's local time zone is used instead.
 +
 `--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''.
+e.g. ``2 hours ago''. The `-local` option cannot be used with
+`--raw` or `--relative`.
 +
-`--date=local` shows timestamps in user's local time zone.
+`--date=local` is an alias for `--date=default-local`.
 +
 `--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
 The differences to the strict ISO 8601 format are:
@@ -730,10 +734,15 @@ format, often found in email messages.
 `--date=format:...` feeds the format `...` to your system `strftime`.
 Use `--date=format:%c` to show the date in your system locale's
 preferred format.  See the `strftime` manual for a complete list of
-format placeholders.
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:...`.
 +
-`--date=default` shows timestamps in the original time zone
-(either committer's or author's).
+`--date=default` is the default format, and is similar to
+`--date=rfc2822`, with a few exceptions:
+
+   - there is no comma after the day-of-week
+
+   - the time zone is omitted when the local time zone is used
 
 ifdef::git-rev-list[]
 --header::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2600,7 +2600,6 @@ parse_done:
   fewer display columns. */
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 
1; /* add the null */
break;
-   case DATE_LOCAL:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -1091,7 +1091,6 @@ struct date_mode {
DATE_NORMAL = 0,
DATE_RELATIVE,
DATE_SHORT,
-   DATE_LOCAL,
DATE_ISO8601,
DATE_ISO8601_STRICT,
DATE_RFC2822,
@@ -1099,6 +1098,7 @@ struct date_mode {
DATE_RAW
} type;
const char *strftime_fmt;
+   int local;
 };
 
 /*
diff --git a/date.c b/date.c
index 8f91569..f048416 100644
--- a/date.c
+++ b/date.c
@@ -166,6 +166,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type 
type)
if (type == DATE_STRFTIME)
die("BUG: cannot create anonymous strftime date_mode struct");
mode.type = type;
+   mode.local = 0;
return &mode;
 }
 
@@ -189,7 +190,7 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
return timebuf.buf;
}
 
-   if (mode->type == DATE_LOCAL)
+   if (mode->local)
tz = local_tzoffset(time);
 
tm = time_to_tm

[PATCH v2 3/6] t6300: introduce test_date() helper

[John Keeping]

This moves the setup of the "expected" file inside the test case.  The
helper function has the advantage that we can use SQ in the file content
without needing to escape the quotes.

Signed-off-by: John Keeping 
---
I considered moving the test_expect_success into the helper, like with
test_atom earlier in the file, but it doesn't make the code much more
concise and we still need either to setup the output outside the test
case or to escape SQ inside SQ.

 t/t6300-for-each-ref.sh | 73 ++---
 1 file changed, 21 insertions(+), 52 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..5fdb964 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -146,85 +146,54 @@ test_expect_success 'Check invalid format specifiers are 
errors' '
test_must_fail git for-each-ref --format="%(authordate:INVALID)" 
refs/heads
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006 
+0200'
-'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
-EOF
+test_date () {
+   f=$1
+   committer_date=$2 &&
+   author_date=$3 &&
+   tagger_date=$4 &&
+   cat >expected <<-EOF &&
+   'refs/heads/master' '$committer_date' '$author_date'
+   'refs/tags/testtag' '$tagger_date'
+   EOF
+   (
+   git for-each-ref --shell --format="%(refname) 
%(committerdate${f:+:$f}) %(authordate${f:+:$f})" refs/heads &&
+   git for-each-ref --shell --format="%(refname) 
%(taggerdate${f:+:$f})" refs/tags
+   ) >actual &&
+   test_cmp expected actual
+}
 
 test_expect_success 'Check unformatted date fields output' '
-   (git for-each-ref --shell --format="%(refname) %(committerdate) 
%(authordate)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) 
>actual &&
-   test_cmp expected actual
+   test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 
+0200" "Mon Jul 3 17:18:45 2006 +0200"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-   f=default &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 
2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
 # doesn't exit in error
-#
-#cat >expected <<\EOF
-#
-#EOF
-#
 test_expect_success 'Check format "relative" date fields output' '
f=relative &&
(git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03' '2006-07-03'
-'refs/tags/testtag' '2006-07-03'
-EOF
-
 test_expect_success 'Check format "short" date fields output' '
-   f=short &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date short 2006-07-03 2006-07-03 2006-07-03
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006'
-'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006'
-EOF
-
 test_expect_success 'Check format "local" date fields output' '
-   f=local &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date local "Mon Jul 3 15:18:43 2006" "Mon Jul 3 15:18:44 2006" 
"Mo

[PATCH v2 5/6] t6300: add test for "raw" date format

[John Keeping]

Signed-off-by: John Keeping 
---
 t/t6300-for-each-ref.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9e0096f..2e76ca9 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -196,6 +196,10 @@ test_expect_success 'Check format "rfc2822" date fields 
output' '
test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 
01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "raw" date fields output' '
+   test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
+'
+
 test_expect_success 'Check format of strftime date fields' '
echo "my date is 2006-07-04" >expected &&
git for-each-ref \
-- 
2.5.0.466.g9af26fa

--
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 4/6] t6300: make UTC and local dates different

[John Keeping]

By setting the UTC time to 23:18:43 the date in +0200 is the following
day, 2006-07-04.  This will ensure that the test for "short-local" to be
added in a following patch tests for different output from the "short"
format.

Signed-off-by: John Keeping 
---
 t/t6300-for-each-ref.sh | 48 
 1 file changed, 24 insertions(+), 24 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 5fdb964..9e0096f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,8 +8,8 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-# Mon Jul 3 15:18:43 2006 +
-datestamp=1151939923
+# Mon Jul 3 23:18:43 2006 +
+datestamp=1151968723
 setdate_and_increment () {
 GIT_COMMITTER_DATE="$datestamp +0200"
 datestamp=$(expr "$datestamp" + 1)
@@ -61,21 +61,21 @@ test_atom head object ''
 test_atom head type ''
 test_atom head '*objectname' ''
 test_atom head '*objecttype' ''
-test_atom head author 'A U Thor  1151939924 +0200'
+test_atom head author 'A U Thor  1151968724 +0200'
 test_atom head authorname 'A U Thor'
 test_atom head authoremail ''
-test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
-test_atom head committer 'C O Mitter  1151939923 +0200'
+test_atom head authordate 'Tue Jul 4 01:18:44 2006 +0200'
+test_atom head committer 'C O Mitter  1151968723 +0200'
 test_atom head committername 'C O Mitter'
 test_atom head committeremail ''
-test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head committerdate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head tag ''
 test_atom head tagger ''
 test_atom head taggername ''
 test_atom head taggeremail ''
 test_atom head taggerdate ''
-test_atom head creator 'C O Mitter  1151939923 +0200'
-test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head creator 'C O Mitter  1151968723 +0200'
+test_atom head creatordate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head subject 'Initial'
 test_atom head contents:subject 'Initial'
 test_atom head body ''
@@ -96,7 +96,7 @@ test_atom tag parent ''
 test_atom tag numparent ''
 test_atom tag object $(git rev-parse refs/tags/testtag^0)
 test_atom tag type 'commit'
-test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag '*objectname' 'ea122842f48be4afb2d1fc6a4b96c05885ab7463'
 test_atom tag '*objecttype' 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
@@ -107,18 +107,18 @@ test_atom tag committername ''
 test_atom tag committeremail ''
 test_atom tag committerdate ''
 test_atom tag tag 'testtag'
-test_atom tag tagger 'C O Mitter  1151939925 +0200'
+test_atom tag tagger 'C O Mitter  1151968725 +0200'
 test_atom tag taggername 'C O Mitter'
 test_atom tag taggeremail ''
-test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag creator 'C O Mitter  1151939925 +0200'
-test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag subject 'Tagging at 1151939927'
-test_atom tag contents:subject 'Tagging at 1151939927'
+test_atom tag taggerdate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter  1151968725 +0200'
+test_atom tag creatordate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151968727'
+test_atom tag contents:subject 'Tagging at 1151968727'
 test_atom tag body ''
 test_atom tag contents:body ''
 test_atom tag contents:signature ''
-test_atom tag contents 'Tagging at 1151939927
+test_atom tag contents 'Tagging at 1151968727
 '
 test_atom tag HEAD ' '
 
@@ -163,11 +163,11 @@ test_date () {
 }
 
 test_expect_success 'Check unformatted date fields output' '
-   test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 
+0200" "Mon Jul 3 17:18:45 2006 +0200"
+   test_date "" "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 2006 
+0200" "Tue Jul 4 01:18:45 2006 +0200"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-   test_date default "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 
2006 +0200" "Mon Jul 3 17:18:45 2006 +0200"
+   test_date default "Tue

[PATCH v2 6/6] t6300: add tests for "-local" date formats

[John Keeping]

Signed-off-by: John Keeping 
---
 t/t6300-for-each-ref.sh | 32 
 1 file changed, 32 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 2e76ca9..c3aee70 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -170,6 +170,10 @@ test_expect_success 'Check format "default" formatted date 
fields output' '
test_date default "Tue Jul 4 01:18:43 2006 +0200" "Tue Jul 4 01:18:44 
2006 +0200" "Tue Jul 4 01:18:45 2006 +0200"
 '
 
+test_expect_success 'Check format "default-local" date fields output' '
+   test_date default-local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 
2006" "Mon Jul 3 23:18:45 2006"
+'
+
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
@@ -180,10 +184,18 @@ test_expect_success 'Check format "relative" date fields 
output' '
git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual
 '
 
+test_expect_success 'Format "relative-local" is not allowed' '
+   test_must_fail git for-each-ref --format="%(authordate:relative-local)" 
refs/heads
+'
+
 test_expect_success 'Check format "short" date fields output' '
test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
+test_expect_success 'Check format "short-local" date fields output' '
+   test_date short-local 2006-07-03 2006-07-03 2006-07-03
+'
+
 test_expect_success 'Check format "local" date fields output' '
test_date local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 2006" 
"Mon Jul 3 23:18:45 2006"
 '
@@ -192,14 +204,26 @@ test_expect_success 'Check format "iso8601" date fields 
output' '
test_date iso8601 "2006-07-04 01:18:43 +0200" "2006-07-04 01:18:44 
+0200" "2006-07-04 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "iso8601-local" date fields output' '
+   test_date iso8601-local "2006-07-03 23:18:43 +" "2006-07-03 
23:18:44 +" "2006-07-03 23:18:45 +"
+'
+
 test_expect_success 'Check format "rfc2822" date fields output' '
test_date rfc2822 "Tue, 4 Jul 2006 01:18:43 +0200" "Tue, 4 Jul 2006 
01:18:44 +0200" "Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "rfc2822-local" date fields output' '
+   test_date rfc2822-local "Mon, 3 Jul 2006 23:18:43 +" "Mon, 3 Jul 
2006 23:18:44 +" "Mon, 3 Jul 2006 23:18:45 +"
+'
+
 test_expect_success 'Check format "raw" date fields output' '
test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
 '
 
+test_expect_success 'Format "raw-local" is not allowed' '
+   test_must_fail git for-each-ref --format="%(authordate:raw-local)" 
refs/heads
+'
+
 test_expect_success 'Check format of strftime date fields' '
echo "my date is 2006-07-04" >expected &&
git for-each-ref \
@@ -208,6 +232,14 @@ test_expect_success 'Check format of strftime date fields' 
'
test_cmp expected actual
 '
 
+test_expect_success 'Check format of strftime-local date fields' '
+   echo "my date is 2006-07-03" >expected &&
+   git for-each-ref \
+ --format="%(authordate:format-local:my date is %Y-%m-%d)" \
+ refs/heads >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'exercise strftime with odd fields' '
echo >expected &&
git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
-- 
2.5.0.466.g9af26fa

--
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/6] date: make "local" orthogonal to date format

[John Keeping]

On Tue, Sep 01, 2015 at 03:16:50PM -0700, Junio C Hamano wrote:
> John Keeping  writes:
> 
> > This is Jeff's original patch with my fixup for DATE_STRFTIME squashed
> > in and a new change to reject "raw-local" (in both Documentation/ and
> > date.c).
> 
> Even in --date=raw, we do show the timezone offset, so I do not
> necessarily agree that raw-local is nonsensical.  That's the only
> difference between the one I queued yesterday and this one.

I suspect it depends on the interpretation of "raw"; the code currently
interprets raw to mean "exactly what exists in the commit/tag", in which
case converting it to the local timezone is wrong.  But the
documentation describes "raw" as "the raw Git %s %z format", and if we
interpret it to mean "Git's internal date format" then "raw-local" makes
sense.

The alternative would be the patch below as a preparatory step.

-- >8 --
diff --git a/date.c b/date.c
index f048416..345890f 100644
--- a/date.c
+++ b/date.c
@@ -175,12 +175,6 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
 
-   if (mode->type == DATE_RAW) {
-   strbuf_reset(&timebuf);
-   strbuf_addf(&timebuf, "%lu %+05d", time, tz);
-   return timebuf.buf;
-   }
-
if (mode->type == DATE_RELATIVE) {
struct timeval now;
 
@@ -193,6 +187,12 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
if (mode->local)
tz = local_tzoffset(time);
 
+   if (mode->type == DATE_RAW) {
+   strbuf_reset(&timebuf);
+   strbuf_addf(&timebuf, "%lu %+05d", time, tz);
+   return timebuf.buf;
+   }
+
tm = time_to_tm(time, tz);
if (!tm) {
tm = time_to_tm(0, 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

Re: [PATCH v2 3/6] t6300: introduce test_date() helper

[John Keeping]

On Tue, Sep 01, 2015 at 06:31:58PM -0400, Jeff King wrote:
> On Tue, Sep 01, 2015 at 10:55:41PM +0100, John Keeping wrote:
> 
> > I considered moving the test_expect_success into the helper, like with
> > test_atom earlier in the file, but it doesn't make the code much more
> > concise and we still need either to setup the output outside the test
> > case or to escape SQ inside SQ.
> 
> Moving it inside also makes it harder to test_expect_failure. :)
> 
> >  test_expect_success 'Check unformatted date fields output' '
> > -   (git for-each-ref --shell --format="%(refname) %(committerdate) 
> > %(authordate)" refs/heads &&
> > -   git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) 
> > >actual &&
> > -   test_cmp expected actual
> > +   test_date "" "Mon Jul 3 17:18:43 2006 +0200" "Mon Jul 3 17:18:44 2006 
> > +0200" "Mon Jul 3 17:18:45 2006 +0200"
> 
> I notice we end up with rather long lines for some of these. Would:
> 
>   test_date "" <<-\EOF
>   Mon Jul 3 17:18:43 2006 +0200
>   Mon Jul 3 17:18:44 2006 +0200
>   Mon Jul 3 17:18:45 2006 +0200
>   EOF
> 
> be more readable?

We could just do:

test_date "" \
"Tue Jul 4 01:18:43 2006 +0200" \
"Tue Jul 4 01:18:44 2006 +0200" \
"Tue Jul 4 01:18:45 2006 +0200"

I'm not entirely sure why I didn't now that I look at it!
--
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 0/6] Make "local" orthogonal to date format

[John Keeping]

On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
> [1] I do think the error message for "relative-local is nonsense" could
> perhaps be more explanatory, but I couldn't come up with any better
> wording. But if you have ideas, feel free to switch it.

My only suggestion would be to reuse the "unknown date format: %s"
message and avoid having a special message in this case.
--
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 0/6] Make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 08:16:59AM -0700, Junio C Hamano wrote:
> Jeff King  writes:
> 
> > On Wed, Sep 02, 2015 at 08:48:26AM +0100, John Keeping wrote:
> >
> >> On Tue, Sep 01, 2015 at 06:44:31PM -0400, Jeff King wrote:
> >> > [1] I do think the error message for "relative-local is nonsense" could
> >> > perhaps be more explanatory, but I couldn't come up with any better
> >> > wording. But if you have ideas, feel free to switch it.
> >> 
> >> My only suggestion would be to reuse the "unknown date format: %s"
> >> message and avoid having a special message in this case.
> >
> > Heh, that was what I was trying to avoid. I wanted to avoid "I do not
> > understand our request" and have it more like "I understand what you're
> > _trying_ to do, but it doesn't make sense".
> >
> > I guess "relative dates do not depend on timezones, so -local is
> > meaningless" would be the closest thing.
> >
> > I don't think it is that big a deal whichever way we go, though.
> 
> I somehow thought that the discussion was about raw-local, not
> relative-local, but anyway, I think it would make more sense to
> allow both of them.  If you define the meaning of "-local" as:
> 
> Pretend that the event in question was recorded with your
> timezone, and show the timestamp using the specified format sans
> -local suffix.
> 
> that explains what happens for all the other formats well, and it
> also makes sense for what would happen to raw and even relative, I
> think.

The discussion about "raw-local" was in a separate subthread, I think
we're just bikeshedding the particular error message here.

OTOH, I don't think there's any disagreement about what "relative-local"
and "raw-local" would output were they supported, just whether they are
useful.  There doesn't seem to be any harm in supporting them;
"relative-local" will be identical to "relative" and "raw-local" will
require preparatory code movement for the raw output.
--
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 0/6] Make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 01:11:35PM -0700, Junio C Hamano wrote:
> John Keeping  writes:
> > OTOH, I don't think there's any disagreement about what "relative-local"
> > and "raw-local" would output were they supported, just whether they are
> > useful.  There doesn't seem to be any harm in supporting them;
> > "relative-local" will be identical to "relative" and "raw-local" will
> > require preparatory code movement for the raw output.
> 
> Sure.
> 
> Bikeshedding further, while Peff's message "-local is meaningless"
> is a correct statement of the fact, I do not think it explains well
> why we chose to error out instead of giving the most natural result
> (i.e. exactly the same as 'relative').
> 
> Perhaps stating "relative-local is not supported" without saying why
> would be better.  "Because it is meaningless, we refuse to support
> the option." is a very strong statement that tells aspiring future
> Git hackers not to attempt to add a support for it, which is
> probably a wrong message to send.

In which case, should we just support it now?

Normally I'd suggest banning controversial options on the basis that
it's easier in the future to allow something that was previously banned
than change the meaning of an options, but in this case I can't see any
other meaning for these options than that described 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 2/2] date: make "local" orthogonal to date format

[John Keeping]

On Wed, Sep 02, 2015 at 05:30:14PM -0400, Jeff King wrote:
> On Wed, Sep 02, 2015 at 10:41:34AM -0700, Junio C Hamano wrote:
> 
> > Jeff King  writes:
> > 
> > > + /* historical alias */
> > > + if (!strcmp(format, "local"))
> > > + format = "default-local";
> > > +
> > > + mode->type = parse_date_type(format, &p);
> > > + mode->local = 0;
> > > +
> > > + if (skip_prefix(p, "-local", &p)) {
> > > + if (mode->type == DATE_RELATIVE)
> > > + die("relative-local date format is nonsensical");
> > > + mode->local = 1;
> > > + }
> > 
> > I notice that we give something funny like this:
> > 
> > $ git show --date=short-locale
> > fatal: unknown date-mode modifier: e
> 
> Yeah, that's not ideal.
> 
> > What is the intention here?  In other words, what kind of things can
> > plausibly follow "--date=short-local" in enhanced versions of Git in
> > the future?  "--date=short-local:some other magic"?
> 
> I had assumed it would be "short-local-othermagic", since ":" is already
> the separator for "format:". But I admit I have no idea what other
> modifiers would be interesting.
> 
> I think the error message would be a lot nicer if we indicate that "-"
> is syntactically interesting, and say:
> 
>   fatal: unknown date-mode modifier: locale

I wonder if we'd be better just saying:

fatal: unknown date format: short-locale

I'm not sure users will consider "local" to be a modifier, there is
simply a list of formats that happens to include pairs of matching
"-local" and "not -local" variants.

That has the benefit of keeping the code simple, otherwise we have to
worry about "shorter" as well (in the patch as it stands that gives
"unknown date-mode modifier: er").
--
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] git-p4: add option to store files in Git LFS on import

[John Keeping]

On Thu, Sep 03, 2015 at 11:40:20AM +0200, Lars Schneider wrote:
> 
> On 30 Aug 2015, at 18:36, Luke Diamand  wrote:
> 
> > On 30 August 2015 at 11:18, Lars Schneider  wrote:
> >> Thanks for your feedback!
> >> 
> >> I like the “handle big files” plugin kind of idea. However, I
> >> wonder if it makes sense to put more and more stuff into git-p4.py
> >> (>3000 LOC already). What do you think about splitting git-p4 into
> >> multiple files?
> > 
> > I was wondering about that. I think for now, the simplicity of keeping
> > everything in one file is worth the slight extra pain. I don't imagine
> > that the big-file-handler code would be very large.
> OK.
> 
> > 
> >> 
> >> Regarding Python 3:
> >> Would you drop Python 2 support or do you want to support Python
> >> 2/3 in parallel? I would prefer the former…
> > 
> > For quite some time we would need to support both; we can't just have
> > a release of git that one day breaks git-p4 for people stuck on Python
> > 2. But it might not be that hard to support both (though converting
> > all those print statements could be quite tiresome).
> Agreed. However supporting both versions increases code complexity as
> well as testing effort. Would a compromise like the following work? We
> fork “git-p4.py” to “git-p4-python2.py” and just apply important bug
> fixes to that file. All new development happens on a Python 3 only
> git-p4.py. 

Documentation/CodingGuidelines currently says:

 - As a minimum, we aim to be compatible with Python 2.6 and 2.7.

 - Where required libraries do not restrict us to Python 2, we try to
   also be compatible with Python 3.1 and later.

That was added in commit 9ef43dd (CodingGuidelines: add Python coding
guidelines, 2013-01-30), which gives the following rationale in the
commit message:

 - Advocating Python 3 support in all scripts is currently unrealistic
   because:

 - 'p4 -G' provides output in a format that is very hard to use with
   Python 3 (and its documentation claims Python 3 is unsupported).

Has that changed?

I also found a message describing why the output is hard to use with
Python 3:

http://permalink.gmane.org/gmane.comp.version-control.git/213316

If that problem can be solved, I don't think it would be difficult to
support 2.6+ and 3.x with a single file.
--
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 00/11] Make "local" orthogonal to date format

[John Keeping]

Since version 2 there are four new preparatory patches which remove
lists of date formats from documentation in favour of referring to the
detailed list in git-rev-list(1) or git-log(1) (both generated from
Documentation/rev-list-options.txt) depending on whether the page in
question is plumbing/porcelain.

I've also reordered the test cleanup patches earlier so that the test
for "--date=raw" is added before the new patch that moves "local"
processing before the "raw" case.  The tests also now wrap long lines
and a missing "&&" has been added.

In patch 7 (date: check for "local" before anything else), we no longer
reject "relative-local" and "raw-local" now prints the user's local
timezone offset.  The error message for invalid formats that are
prefixed with a valid format name is now the same as that if there is no
valid prefix.

Jeff King (2):
  fast-import: switch crash-report date to iso8601
  date: make "local" orthogonal to date format

John Keeping (9):
  Documentation/blame-options: don't list date formats
  Documentation/config: don't list date formats
  Documentation/git-for-each-ref: don't list date formats
  Documentation/rev-list: don't list date formats
  t6300: introduce test_date() helper
  t6300: add test for "raw" date format
  date: check for "local" before anything else
  t6300: make UTC and local dates different
  t6300: add tests for "-local" date formats

 Documentation/blame-options.txt|   5 +-
 Documentation/config.txt   |   4 +-
 Documentation/git-for-each-ref.txt |   5 +-
 Documentation/git-rev-list.txt |   2 +-
 Documentation/rev-list-options.txt |  23 --
 builtin/blame.c|   1 -
 cache.h|   2 +-
 date.c |  74 ++---
 fast-import.c  |   2 +-
 t/t6300-for-each-ref.sh| 162 ++---
 10 files changed, 166 insertions(+), 114 deletions(-)

-- 
2.5.0.466.g9af26fa

--
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 01/11] Documentation/blame-options: don't list date formats

[John Keeping]

This list is already incomplete (missing "raw") and we're about to add
new formats.  Remove it and refer to the canonical documentation in
git-log(1).

Signed-off-by: John Keeping 
---
 Documentation/blame-options.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index a09969b..760eab7 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -63,11 +63,10 @@ include::line-range-format.txt[]
`-` to make the command read from the standard input).
 
 --date ::
-   The value is one of the following alternatives:
-   {relative,local,default,iso,rfc,short}. If --date is not
+   Specifies the format used to output dates. If --date is not
provided, the value of the blame.date config variable is
used. If the blame.date config variable is also not set, the
-   iso format is used. For more information, See the discussion
+   iso format is used. For supported values, see the discussion
of the --date option at linkgit:git-log[1].
 
 -M||::
-- 
2.5.0.466.g9af26fa

--
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 02/11] Documentation/config: don't list date formats

[John Keeping]

This list is already incomplete (missing "raw") and we're about to add
new formats.  Since this option sets a default for git-log's --date
option, just refer to git-log(1).

Signed-off-by: John Keeping 
---
 Documentation/config.txt | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f5d15ff..49f9fa8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1829,9 +1829,7 @@ log.abbrevCommit::
 log.date::
Set the default date-time mode for the 'log' command.
Setting a value for log.date is similar to using 'git log''s
-   `--date` option.  Possible values are `relative`, `local`,
-   `default`, `iso`, `rfc`, and `short`; see linkgit:git-log[1]
-   for details.
+   `--date` option.  See linkgit:git-log[1] for details.
 
 log.decorate::
Print out the ref names of any commits that are shown by the log
-- 
2.5.0.466.g9af26fa

--
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 03/11] Documentation/git-for-each-ref: don't list date formats

[John Keeping]

We are about to add a new set of supported date formats and do not want
to have to maintain the same list in several different bits of
documentation.  Refer to git-rev-list(1) which contains the full list of
supported formats.

Signed-off-by: John Keeping 
---
 Documentation/git-for-each-ref.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 7f8d9a5..d062639 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -134,9 +134,8 @@ the object referred by the ref does not cause an error.  It
 returns an empty string instead.
 
 As a special case for the date-type fields, you may specify a format for
-the date by adding one of `:default`, `:relative`, `:short`, `:local`,
-`:iso8601`, `:rfc2822` or `:raw` to the end of the fieldname; e.g.
-`%(taggerdate:relative)`.
+the date by adding `:` followed by date format name (see the
+values the `--date` option to linkgit::git-rev-list[1] takes).
 
 
 EXAMPLES
-- 
2.5.0.466.g9af26fa

--
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 05/11] fast-import: switch crash-report date to iso8601

[John Keeping]

From: Jeff King 

When fast-import emits a crash report, it does so in the
user's local timezone. But because we omit the timezone
completely for DATE_LOCAL, a reader of the report does not
immediately know which time zone was used. Let's switch this
to ISO8601 instead, which includes the time zone.

This does mean we will show the time in UTC, but that's not
a big deal. A crash report like this will either be looked
at immediately (in which case nobody even looks at the
timestamp), or it will be passed along to a developer to
debug, in which case the original timezone is less likely to
be of interest.

Signed-off-by: Jeff King 
Signed-off-by: John Keeping 
---
 fast-import.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 6c7c3c9..adcbfc6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -424,7 +424,7 @@ static void write_crash_report(const char *err)
fprintf(rpt, "fast-import crash report:\n");
fprintf(rpt, "fast-import process: %"PRIuMAX"\n", (uintmax_t) 
getpid());
fprintf(rpt, "parent process : %"PRIuMAX"\n", (uintmax_t) 
getppid());
-   fprintf(rpt, "at %s\n", show_date(time(NULL), 0, DATE_MODE(LOCAL)));
+   fprintf(rpt, "at %s\n", show_date(time(NULL), 0, 
DATE_MODE(ISO8601)));
fputc('\n', rpt);
 
fputs("fatal: ", rpt);
-- 
2.5.0.466.g9af26fa

--
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 04/11] Documentation/rev-list: don't list date formats

[John Keeping]

We are about to add several new date formats which will make this list
too long to display in a single line.

Signed-off-by: John Keeping 
---
 Documentation/git-rev-list.txt | 2 +-
 Documentation/rev-list-options.txt | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rev-list.txt b/Documentation/git-rev-list.txt
index 7b49c85..ef22f17 100644
--- a/Documentation/git-rev-list.txt
+++ b/Documentation/git-rev-list.txt
@@ -45,7 +45,7 @@ SYNOPSIS
 [ --regexp-ignore-case | -i ]
 [ --extended-regexp | -E ]
 [ --fixed-strings | -F ]
-[ --date=(local|relative|default|iso|iso-strict|rfc|short) ]
+[ --date=]
 [ [ --objects | --objects-edge | --objects-edge-aggressive ]
   [ --unpacked ] ]
 [ --pretty | --header ]
diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index a9b808f..14c4cce 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -699,7 +699,7 @@ include::pretty-options.txt[]
 --relative-date::
Synonym for `--date=relative`.
 
---date=(relative|local|default|iso|iso-strict|rfc|short|raw)::
+--date=::
Only takes effect for dates shown in human-readable format, such
as when using `--pretty`. `log.date` config variable sets a default
value for the log command's `--date` option.
-- 
2.5.0.466.g9af26fa

--
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 07/11] t6300: add test for "raw" date format

[John Keeping]

Signed-off-by: John Keeping 
---
 t/t6300-for-each-ref.sh | 4 
 1 file changed, 4 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 0bf709b..6afcff6 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -215,6 +215,10 @@ test_expect_success 'Check format "rfc2822" date fields 
output' '
"Mon, 3 Jul 2006 17:18:45 +0200"
 '
 
+test_expect_success 'Check format "raw" date fields output' '
+   test_date raw "1151939923 +0200" "1151939924 +0200" "1151939925 +0200"
+'
+
 test_expect_success 'Check format of strftime date fields' '
echo "my date is 2006-07-03" >expected &&
git for-each-ref \
-- 
2.5.0.466.g9af26fa

--
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 06/11] t6300: introduce test_date() helper

[John Keeping]

This moves the setup of the "expected" file inside the test case.  The
helper function has the advantage that we can use SQ in the file content
without needing to escape the quotes.

Signed-off-by: John Keeping 
---
Changes since v2:
- add missing "&&" after "f=$1"
- wrap long lines

 t/t6300-for-each-ref.sh | 92 +
 1 file changed, 40 insertions(+), 52 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 7c9bec7..0bf709b 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -146,85 +146,73 @@ test_expect_success 'Check invalid format specifiers are 
errors' '
test_must_fail git for-each-ref --format="%(authordate:INVALID)" 
refs/heads
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 17:18:43 2006 +0200' 'Mon Jul 3 17:18:44 2006 
+0200'
-'refs/tags/testtag' 'Mon Jul 3 17:18:45 2006 +0200'
-EOF
+test_date () {
+   f=$1 &&
+   committer_date=$2 &&
+   author_date=$3 &&
+   tagger_date=$4 &&
+   cat >expected <<-EOF &&
+   'refs/heads/master' '$committer_date' '$author_date'
+   'refs/tags/testtag' '$tagger_date'
+   EOF
+   (
+   git for-each-ref --shell \
+   --format="%(refname) %(committerdate${f:+:$f}) 
%(authordate${f:+:$f})" \
+   refs/heads &&
+   git for-each-ref --shell \
+   --format="%(refname) %(taggerdate${f:+:$f})" \
+   refs/tags
+   ) >actual &&
+   test_cmp expected actual
+}
 
 test_expect_success 'Check unformatted date fields output' '
-   (git for-each-ref --shell --format="%(refname) %(committerdate) 
%(authordate)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate)" refs/tags) 
>actual &&
-   test_cmp expected actual
+   test_date "" \
+   "Mon Jul 3 17:18:43 2006 +0200" \
+   "Mon Jul 3 17:18:44 2006 +0200" \
+   "Mon Jul 3 17:18:45 2006 +0200"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
-   f=default &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date default \
+   "Mon Jul 3 17:18:43 2006 +0200" \
+   "Mon Jul 3 17:18:44 2006 +0200" \
+   "Mon Jul 3 17:18:45 2006 +0200"
 '
 
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
 # doesn't exit in error
-#
-#cat >expected <<\EOF
-#
-#EOF
-#
 test_expect_success 'Check format "relative" date fields output' '
f=relative &&
(git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual
 '
 
-cat >expected <<\EOF
-'refs/heads/master' '2006-07-03' '2006-07-03'
-'refs/tags/testtag' '2006-07-03'
-EOF
-
 test_expect_success 'Check format "short" date fields output' '
-   f=short &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date short 2006-07-03 2006-07-03 2006-07-03
 '
 
-cat >expected <<\EOF
-'refs/heads/master' 'Mon Jul 3 15:18:43 2006' 'Mon Jul 3 15:18:44 2006'
-'refs/tags/testtag' 'Mon Jul 3 15:18:45 2006'
-EOF
-
 test_expect_success 'Check format "local" date fields output' '
-   f=local &&
-   (git for-each-ref --shell --format="%(refname) %(committerdate:$f) 
%(authordate:$f)" refs/heads &&
-   git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual &&
-   test_cmp expected actual
+   test_date local \
+ 

[PATCH v3 08/11] date: check for "local" before anything else

[John Keeping]

In a following commit we will make "local" orthogonal to the format.
Although this will not apply to "relative", which does not use the
timezone, it applies to all other formats so move the timezone
conversion to the start of the function.

Signed-off-by: John Keeping 
---
 date.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/date.c b/date.c
index 8f91569..9f0a5dd 100644
--- a/date.c
+++ b/date.c
@@ -174,6 +174,9 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
struct tm *tm;
static struct strbuf timebuf = STRBUF_INIT;
 
+   if (mode->type == DATE_LOCAL)
+   tz = local_tzoffset(time);
+
if (mode->type == DATE_RAW) {
strbuf_reset(&timebuf);
strbuf_addf(&timebuf, "%lu %+05d", time, tz);
@@ -189,9 +192,6 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
return timebuf.buf;
}
 
-   if (mode->type == DATE_LOCAL)
-   tz = local_tzoffset(time);
-
tm = time_to_tm(time, tz);
if (!tm) {
tm = time_to_tm(0, 0);
-- 
2.5.0.466.g9af26fa

--
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 09/11] date: make "local" orthogonal to date format

[John Keeping]

From: Jeff King 

Most of our "--date" modes are about the format of the date:
which items we show and in what order. But "--date=local" is
a bit of an oddball. It means "show the date in the normal
format, but using the local timezone". The timezone we use
is orthogonal to the actual format, and there is no reason
we could not have "localized iso8601", etc.

This patch adds a "local" boolean field to "struct
date_mode", and drops the DATE_LOCAL element from the
date_mode_type enum (it's now just DATE_NORMAL plus
local=1). The new feature is accessible to users by adding
"-local" to any date mode (e.g., "iso-local"), and we retain
"local" as an alias for "default-local" for backwards
compatibility.

Signed-off-by: Jeff King 
Signed-off-by: John Keeping 
---
Changes since v2:
- "local" check has moved above DATE_RAW processing as a result of an
  earlier patch
- "relative-local" and "raw-local" are now allowed
- the error message if the format starts with a valid sequence but is
  invalid as a whole is now consistent with the case where there is no
  valid prefix

 Documentation/rev-list-options.txt | 21 
 builtin/blame.c|  1 -
 cache.h|  2 +-
 date.c | 70 --
 4 files changed, 61 insertions(+), 33 deletions(-)

diff --git a/Documentation/rev-list-options.txt 
b/Documentation/rev-list-options.txt
index 14c4cce..359587c 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -702,12 +702,16 @@ include::pretty-options.txt[]
 --date=::
Only takes effect for dates shown in human-readable format, such
as when using `--pretty`. `log.date` config variable sets a default
-   value for the log command's `--date` option.
+   value for the log command's `--date` option. By default, dates
+   are shown in the original time zone (either committer's or
+   author's). If `-local` is appended to the format (e.g.,
+   `iso-local`), the user's local time zone is used instead.
 +
 `--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''.
+e.g. ``2 hours ago''. The `-local` option cannot be used with
+`--raw` or `--relative`.
 +
-`--date=local` shows timestamps in user's local time zone.
+`--date=local` is an alias for `--date=default-local`.
 +
 `--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
 The differences to the strict ISO 8601 format are:
@@ -730,10 +734,15 @@ format, often found in email messages.
 `--date=format:...` feeds the format `...` to your system `strftime`.
 Use `--date=format:%c` to show the date in your system locale's
 preferred format.  See the `strftime` manual for a complete list of
-format placeholders.
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:...`.
 +
-`--date=default` shows timestamps in the original time zone
-(either committer's or author's).
+`--date=default` is the default format, and is similar to
+`--date=rfc2822`, with a few exceptions:
+
+   - there is no comma after the day-of-week
+
+   - the time zone is omitted when the local time zone is used
 
 ifdef::git-rev-list[]
 --header::
diff --git a/builtin/blame.c b/builtin/blame.c
index 4db01c1..6fd1a63 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2600,7 +2600,6 @@ parse_done:
   fewer display columns. */
blame_date_width = utf8_strwidth(_("4 years, 11 months ago")) + 
1; /* add the null */
break;
-   case DATE_LOCAL:
case DATE_NORMAL:
blame_date_width = sizeof("Thu Oct 19 16:00:04 2006 -0700");
break;
diff --git a/cache.h b/cache.h
index 4e25271..9a91b1d 100644
--- a/cache.h
+++ b/cache.h
@@ -1091,7 +1091,6 @@ struct date_mode {
DATE_NORMAL = 0,
DATE_RELATIVE,
DATE_SHORT,
-   DATE_LOCAL,
DATE_ISO8601,
DATE_ISO8601_STRICT,
DATE_RFC2822,
@@ -1099,6 +1098,7 @@ struct date_mode {
DATE_RAW
} type;
const char *strftime_fmt;
+   int local;
 };
 
 /*
diff --git a/date.c b/date.c
index 9f0a5dd..7c9f769 100644
--- a/date.c
+++ b/date.c
@@ -166,6 +166,7 @@ struct date_mode *date_mode_from_type(enum date_mode_type 
type)
if (type == DATE_STRFTIME)
die("BUG: cannot create anonymous strftime date_mode struct");
mode.type = type;
+   mode.local = 0;
return &mode;
 }
 
@@ -174,7 +175,7 @@ const char *show_date(unsigned long time, int tz, const 
struct date_mode *mode)
struct tm *tm;
static struct strbuf timebuf = STRBUF_INI

[PATCH v3 11/11] t6300: add tests for "-local" date formats

[John Keeping]

Signed-off-by: John Keeping 
---
Changes since v2:
- "relative-local" and "raw-local" are now allowed

 t/t6300-for-each-ref.sh | 36 
 1 file changed, 36 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 899251e..03873b0 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -180,6 +180,10 @@ test_expect_success 'Check format "default" formatted date 
fields output' '
"Tue Jul 4 01:18:45 2006 +0200"
 '
 
+test_expect_success 'Check format "default-local" date fields output' '
+   test_date default-local "Mon Jul 3 23:18:43 2006" "Mon Jul 3 23:18:44 
2006" "Mon Jul 3 23:18:45 2006"
+'
+
 # Don't know how to do relative check because I can't know when this script
 # is going to be run and can't fake the current time to git, and hence can't
 # provide expected output.  Instead, I'll just make sure that "relative"
@@ -190,10 +194,22 @@ test_expect_success 'Check format "relative" date fields 
output' '
git for-each-ref --shell --format="%(refname) %(taggerdate:$f)" 
refs/tags) >actual
 '
 
+# We just check that this is the same as "relative" for now.
+test_expect_success 'Check format "relative-local" date fields output' '
+   test_date relative-local \
+   "$(git for-each-ref --format="%(committerdate:relative)" 
refs/heads)" \
+   "$(git for-each-ref --format="%(authordate:relative)" 
refs/heads)" \
+   "$(git for-each-ref --format="%(taggerdate:relative)" 
refs/tags)"
+'
+
 test_expect_success 'Check format "short" date fields output' '
test_date short 2006-07-04 2006-07-04 2006-07-04
 '
 
+test_expect_success 'Check format "short-local" date fields output' '
+   test_date short-local 2006-07-03 2006-07-03 2006-07-03
+'
+
 test_expect_success 'Check format "local" date fields output' '
test_date local \
"Mon Jul 3 23:18:43 2006" \
@@ -208,6 +224,10 @@ test_expect_success 'Check format "iso8601" date fields 
output' '
"2006-07-04 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "iso8601-local" date fields output' '
+   test_date iso8601-local "2006-07-03 23:18:43 +" "2006-07-03 
23:18:44 +" "2006-07-03 23:18:45 +"
+'
+
 test_expect_success 'Check format "rfc2822" date fields output' '
test_date rfc2822 \
"Tue, 4 Jul 2006 01:18:43 +0200" \
@@ -215,10 +235,18 @@ test_expect_success 'Check format "rfc2822" date fields 
output' '
"Tue, 4 Jul 2006 01:18:45 +0200"
 '
 
+test_expect_success 'Check format "rfc2822-local" date fields output' '
+   test_date rfc2822-local "Mon, 3 Jul 2006 23:18:43 +" "Mon, 3 Jul 
2006 23:18:44 +" "Mon, 3 Jul 2006 23:18:45 +"
+'
+
 test_expect_success 'Check format "raw" date fields output' '
test_date raw "1151968723 +0200" "1151968724 +0200" "1151968725 +0200"
 '
 
+test_expect_success 'Check format "raw-local" date fields output' '
+   test_date raw-local "1151968723 +" "1151968724 +" "1151968725 
+"
+'
+
 test_expect_success 'Check format of strftime date fields' '
echo "my date is 2006-07-04" >expected &&
git for-each-ref \
@@ -227,6 +255,14 @@ test_expect_success 'Check format of strftime date fields' 
'
test_cmp expected actual
 '
 
+test_expect_success 'Check format of strftime-local date fields' '
+   echo "my date is 2006-07-03" >expected &&
+   git for-each-ref \
+ --format="%(authordate:format-local:my date is %Y-%m-%d)" \
+ refs/heads >actual &&
+   test_cmp expected actual
+'
+
 test_expect_success 'exercise strftime with odd fields' '
echo >expected &&
git for-each-ref --format="%(authordate:format:)" refs/heads >actual &&
-- 
2.5.0.466.g9af26fa

--
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 10/11] t6300: make UTC and local dates different

[John Keeping]

By setting the UTC time to 23:18:43 the date in +0200 is the following
day, 2006-07-04.  This will ensure that the test for "short-local" to be
added in the following patch tests for different output from the "short"
format.

Signed-off-by: John Keeping 
---
 t/t6300-for-each-ref.sh | 70 -
 1 file changed, 35 insertions(+), 35 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 6afcff6..899251e 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,8 +8,8 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-gpg.sh
 
-# Mon Jul 3 15:18:43 2006 +
-datestamp=1151939923
+# Mon Jul 3 23:18:43 2006 +
+datestamp=1151968723
 setdate_and_increment () {
 GIT_COMMITTER_DATE="$datestamp +0200"
 datestamp=$(expr "$datestamp" + 1)
@@ -61,21 +61,21 @@ test_atom head object ''
 test_atom head type ''
 test_atom head '*objectname' ''
 test_atom head '*objecttype' ''
-test_atom head author 'A U Thor  1151939924 +0200'
+test_atom head author 'A U Thor  1151968724 +0200'
 test_atom head authorname 'A U Thor'
 test_atom head authoremail ''
-test_atom head authordate 'Mon Jul 3 17:18:44 2006 +0200'
-test_atom head committer 'C O Mitter  1151939923 +0200'
+test_atom head authordate 'Tue Jul 4 01:18:44 2006 +0200'
+test_atom head committer 'C O Mitter  1151968723 +0200'
 test_atom head committername 'C O Mitter'
 test_atom head committeremail ''
-test_atom head committerdate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head committerdate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head tag ''
 test_atom head tagger ''
 test_atom head taggername ''
 test_atom head taggeremail ''
 test_atom head taggerdate ''
-test_atom head creator 'C O Mitter  1151939923 +0200'
-test_atom head creatordate 'Mon Jul 3 17:18:43 2006 +0200'
+test_atom head creator 'C O Mitter  1151968723 +0200'
+test_atom head creatordate 'Tue Jul 4 01:18:43 2006 +0200'
 test_atom head subject 'Initial'
 test_atom head contents:subject 'Initial'
 test_atom head body ''
@@ -96,7 +96,7 @@ test_atom tag parent ''
 test_atom tag numparent ''
 test_atom tag object $(git rev-parse refs/tags/testtag^0)
 test_atom tag type 'commit'
-test_atom tag '*objectname' '67a36f10722846e891fbada1ba48ed035de75581'
+test_atom tag '*objectname' 'ea122842f48be4afb2d1fc6a4b96c05885ab7463'
 test_atom tag '*objecttype' 'commit'
 test_atom tag author ''
 test_atom tag authorname ''
@@ -107,18 +107,18 @@ test_atom tag committername ''
 test_atom tag committeremail ''
 test_atom tag committerdate ''
 test_atom tag tag 'testtag'
-test_atom tag tagger 'C O Mitter  1151939925 +0200'
+test_atom tag tagger 'C O Mitter  1151968725 +0200'
 test_atom tag taggername 'C O Mitter'
 test_atom tag taggeremail ''
-test_atom tag taggerdate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag creator 'C O Mitter  1151939925 +0200'
-test_atom tag creatordate 'Mon Jul 3 17:18:45 2006 +0200'
-test_atom tag subject 'Tagging at 1151939927'
-test_atom tag contents:subject 'Tagging at 1151939927'
+test_atom tag taggerdate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag creator 'C O Mitter  1151968725 +0200'
+test_atom tag creatordate 'Tue Jul 4 01:18:45 2006 +0200'
+test_atom tag subject 'Tagging at 1151968727'
+test_atom tag contents:subject 'Tagging at 1151968727'
 test_atom tag body ''
 test_atom tag contents:body ''
 test_atom tag contents:signature ''
-test_atom tag contents 'Tagging at 1151939927
+test_atom tag contents 'Tagging at 1151968727
 '
 test_atom tag HEAD ' '
 
@@ -168,16 +168,16 @@ test_date () {
 
 test_expect_success 'Check unformatted date fields output' '
test_date "" \
-   "Mon Jul 3 17:18:43 2006 +0200" \
-   "Mon Jul 3 17:18:44 2006 +0200" \
-   "Mon Jul 3 17:18:45 2006 +0200"
+   "Tue Jul 4 01:18:43 2006 +0200" \
+   "Tue Jul 4 01:18:44 2006 +0200" \
+   "Tue Jul 4 01:18:45 2006 +0200"
 '
 
 test_expect_success 'Check format "default" formatted date fields output' '
test_date default \
-   "Mon Jul 3 17:18:43 2006 +0200" \
-   "Mon J

Re: determine name of tag used for checkout when multiple tags exist?

[John Keeping]

On Thu, Sep 03, 2015 at 08:53:16PM -0600, Jesse Hopkins wrote:
> Looking for suggestions on how to determine the tag that was used to
> checkout a git repo to its associated commit, particularly in the case
> where multiple tags might point to the same commit.
> 
> I've had a look at git-name-rev and git-describe, and both seem useful so
> long as there's only one tag pointing to the commit of interest.  However,
> I'm still coming up to speed on their behavior in the multiple tag case
> (mainly by experimentation).
> 
> It seems to me that when checking out to a tag, Git does not record the
> *name* of the tag anywhere, but rather sets HEAD to the de-referenced
> commit SHA-1.  As far as I can tell, it is not possible to recover the
> original name of the tag in the case of multiple tags on the same commit.
> Is my conclusion correct?
> 
> The reason I ask is that we have a build environment where it is likely
> that multiple tags will get set by various groups in our main 'truth' Git
> repo.  We are using some scripting that would like to know the *name* of
> the tag used for checkout (this has been working well for us so far as long
> as we checkout against branches).
> 
> Is there perhaps some other means of doing a checkout to tag that DOES
> record the name of the tag?  If not, I imagine we might need some external
> means to record the checked out tag, which is not out of the question.

Have you considered looking in the reflog?

When I checkout a tag, "git reflog -1" gives something like:

989d251 HEAD@{0}: checkout: moving from master to v0.9.2

Since whitespace isn't permitted in tag names you can do something like:

tag=$(git reflog -1)
tag=${tag##* }
git cat-file tag "$tag" >/dev/null 2>&1 || echo "not a tag!"
--
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: determine name of tag used for checkout when multiple tags exist?

[John Keeping]

[It looks like your reply didn't get through to the mailing list,
 presumably because it contained a text/html part.]

On Fri, Sep 04, 2015 at 04:22:04AM -0600, Jesse Hopkins wrote:
> On Sep 4, 2015 1:54 AM, "John Keeping"  wrote:
> > When I checkout a tag, "git reflog -1" gives something like:
> >
> > 989d251 HEAD@{0}: checkout: moving from master to v0.9.2
> >
> > Since whitespace isn't permitted in tag names you can do something like:
> >
> > tag=$(git reflog -1)
> > tag=${tag##* }
> > git cat-file tag "$tag" >/dev/null 2>&1 || echo "not a tag!"
> 
> Thanks John that seems promising. One limitation it seems is that the
> reflog doesn't contain the tag name on a freshly cloned repo which used the
> tag as the -b option.   However it seems I can recover the tag name from
> the reflog so long as I clone against something other than the tag,  then
> checkout the tag.

I think it would be a reasonable enhancement to include the branch name
in the reflog message if "-b" is given to "git clone", but I'm not aware
of any (formal) policy on the format of reflog messages so relying on
any particular message may not be 100% reliable across Git upgrades.
--
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] test_when_finished in subshells

[John Keeping]

A recent thread [0] made me realise that using test_when_finished in a
subshell is likely to be a bug, since the change to $test_cleanup will
be lost when the subshell exits.

There is no POSIX way to detect that we are in a subshell ($$ and $PPID
are specified to remain unchanged), but we can detect it on Bash and
gracefully fall back to disabling the test on other shells, which is
what the patch below does.

There are three tests currently in master that fail with this patch (at
least on my system):

Test Summary Report
---
t7610-mergetool.sh   (Wstat: 256 Tests: 18 
Failed: 1)
  Failed test:  7
  Non-zero exit status: 1
t7800-difftool.sh(Wstat: 256 Tests: 56 
Failed: 1)
  Failed test:  56
  Non-zero exit status: 1
t5801-remote-helpers.sh  (Wstat: 256 Tests: 30 
Failed: 1)
  Failed test:  27
  Non-zero exit status: 1

All are harmless at the moment and t7610 and t5801 can be fixed by
moving the test_when_finished call out of the subshell relatively
easily.

t7800 (in its final test) calls test_config in a subshell which has cd'd
into a submodule.

Is this something worth worrying about, or is it sufficiently rare that
we can live with the current behaviour?

[0] http://article.gmane.org/gmane.comp.version-control.git/277199

-- >8 --
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..d29cd7b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -722,6 +722,8 @@ test_seq () {
 # what went wrong.
 
 test_when_finished () {
+   test "${BASH_SUBSHELL-0}" = 0 ||
+   error "bug in test script: test_when_finished does nothing in a 
subshell"
test_cleanup="{ $*
} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
-- 
2.5.0.466.g9af26fa

--
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] Makefile: fix MAKEFLAGS tests with multiple flags

[John Keeping]

findstring is defined as $(findstring FIND,IN) so if multiple flags are
set these tests do the wrong thing unless $(MAKEFLAGS) is the second
argument.

Signed-off-by: John Keeping 
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 34101e2..a556be6 100644
--- a/Makefile
+++ b/Makefile
@@ -1463,13 +1463,13 @@ endif
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
-ifneq ($(findstring $(MAKEFLAGS),w),w)
+ifneq ($(findstring w,$(MAKEFLAGS)),w)
 PRINT_DIR = --no-print-directory
 else # "make -w"
 NO_SUBDIR = :
 endif
 
-ifneq ($(findstring $(MAKEFLAGS),s),s)
+ifneq ($(findstring s,$(MAKEFLAGS)),s)
 ifndef V
QUIET_CC   = @echo '   ' CC $@;
QUIET_AR   = @echo '   ' AR $@;
-- 
2.5.0.466.g9af26fa

--
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/5] disallow test_when_finished in subshells

[John Keeping]

On Sat, Sep 05, 2015 at 04:54:30AM -0400, Jeff King wrote:
> On Fri, Sep 04, 2015 at 11:43:15AM -0700, Junio C Hamano wrote:
> 
> > > t7800 (in its final test) calls test_config in a subshell which has cd'd
> > > into a submodule.
> > >
> > > Is this something worth worrying about, or is it sufficiently rare that
> > > we can live with the current behaviour?
> > 
> > Fixing the instances you found is good, obviously ;-).  Thanks for
> > working on this.
> > 
> > Even though the proposed detection is BASH-ism, I think it would not
> > hurt other shells (they obviously do not help you catch bugs, but
> > they would not misbehave as long as you make sure BASH_SUBSHELL is
> > either unset or set to 0 at the beginning of the test), and the only
> > impact to them would be a invocation of (often built-in) 'test'
> > utility, whose performance impact should be miniscule.
> > 
> > I'll wait for opinion from others, of course.
> 
> I like it. In general I'm in favor of any lint-like fixes (whether for
> the tests or the C code itself) as long as:
> 
>   1. they don't create false positive noise
> 
>   2. they don't require extra effort at each call-site
> 
>   3. they don't have a performance impact
> 
> And I think this passes all three. Of course it would be nice if the new
> check ran on all shells, but even this seems like a strict improvement.

Here are the changes to do this.  The first two are pretty
straightforward, but the t7800 change may be more controversial; in this
particular case we could get away with using "git config" instead of
test_config but I think the more generic solution will be better for the
future.

I don't think it's worth trying to clear $BASH_SUBSHELL before the tests
start because to do so we have to reliably detect that we're not running
under Bash, and if we don't trust people not to set $BASH_SUBSHELL why
do we trust them not to set $BASH?


John Keeping (5):
  t7610: don't use test_config in a subshell
  t5801: don't use test_when_finished in a subshell
  test-lib-functions: support "test_config -C  ..."
  t7800: don't use test_config in a subshell
  test-lib-functions: detect test_when_finished in subshell

 t/t5801-remote-helpers.sh | 12 
 t/t7610-mergetool.sh  |  2 +-
 t/t7800-difftool.sh   |  8 
 t/test-lib-functions.sh   | 25 ++---
 4 files changed, 31 insertions(+), 16 deletions(-)

-- 
2.5.0.466.g9af26fa

--
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/5] t5801: don't use test_when_finished in a subshell

[John Keeping]

test_when_finished has no effect in a subshell.  Since the cmp_marks
function is only used once, inline it at its call site and move the
test_when_finished invocation to the start of the test.

Signed-off-by: John Keeping 
---
 t/t5801-remote-helpers.sh | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index c9d3ed1..362b158 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -242,13 +242,6 @@ clean_mark () {
sort >$(basename "$1")
 }
 
-cmp_marks () {
-   test_when_finished "rm -rf git.marks testgit.marks" &&
-   clean_mark ".git/testgit/$1/git.marks" &&
-   clean_mark ".git/testgit/$1/testgit.marks" &&
-   test_cmp git.marks testgit.marks
-}
-
 test_expect_success 'proper failure checks for fetching' '
(cd local &&
test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git fetch 2>error &&
@@ -258,12 +251,15 @@ test_expect_success 'proper failure checks for fetching' '
 '
 
 test_expect_success 'proper failure checks for pushing' '
+   test_when_finished "rm -rf local/git.marks local/testgit.marks" &&
(cd local &&
git checkout -b crash master &&
echo crash >>file &&
git commit -a -m crash &&
test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&
-   cmp_marks origin
+   clean_mark ".git/testgit/origin/git.marks" &&
+   clean_mark ".git/testgit/origin/testgit.marks" &&
+   test_cmp git.marks testgit.marks
)
 '
 
-- 
2.5.0.466.g9af26fa

--
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/5] t7610: don't use test_config in a subshell

[John Keeping]

test_config uses test_when_finished to reset the configuration after the
test, but this does not work inside a subshell.  This does not cause a
problem here because the first thing the next test does is to set this
config variable itself, but we are about to add a check that will
complain when test_when_finished is used in a subshell.

In this case, "subdir" not a submodule so test_config has the same
effect when run at the top level and can simply be moved out of the
subshell.

Signed-off-by: John Keeping 
---
 t/t7610-mergetool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7eeb207..6f12b23 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -174,9 +174,9 @@ test_expect_success 'mergetool skips autoresolved' '
 '
 
 test_expect_success 'mergetool merges all from subdir' '
+   test_config rerere.enabled false &&
(
cd subdir &&
-   test_config rerere.enabled false &&
test_must_fail git merge master &&
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
-- 
2.5.0.466.g9af26fa

--
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 4/5] t7800: don't use test_config in a subshell

[John Keeping]

Use the new "-C" option to test_config to change the configuration in
the submodule from the top level of the test so that it can be unset
correctly when the test finishes.

Signed-off-by: John Keeping 
---
 t/t7800-difftool.sh | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ea35a02..48c6e2b 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -492,12 +492,12 @@ test_expect_success PERL 'difftool --no-symlinks detects 
conflict ' '
 
 test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
git submodule add ./. submod/ule &&
+   test_config -C submod/ule diff.tool checktrees &&
+   test_config -C submod/ule difftool.checktrees.cmd '\''
+   test -d "$LOCAL" && test -d "$REMOTE" && echo good
+   '\'' &&
(
cd submod/ule &&
-   test_config diff.tool checktrees &&
-   test_config difftool.checktrees.cmd '\''
-   test -d "$LOCAL" && test -d "$REMOTE" && echo good
-   '\'' &&
echo good >expect &&
git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
test_cmp expect actual
-- 
2.5.0.466.g9af26fa

--
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/5] test-lib-functions: support "test_config -C ..."

[John Keeping]

If used in a subshell, test_config cannot unset variables at the end of
a test.  This is a problem when testing submodules because we do not
want to "cd" at to top level of a test script in order to run the
command inside the submodule.

Add a "-C" option to test_config (and test_unconfig) so that test_config
can be kept outside subshells and still affect subrepositories.

Signed-off-by: John Keeping 
---
 t/test-lib-functions.sh | 20 +---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..0e80f37 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -201,7 +201,14 @@ test_chmod () {
 
 # Unset a configuration variable, but don't fail if it doesn't exist.
 test_unconfig () {
-   git config --unset-all "$@"
+   config_dir=
+   if test "$1" = -C
+   then
+   shift
+   config_dir=$1
+   shift
+   fi
+   git ${config_dir:+-C "$config_dir"} config --unset-all "$@"
config_status=$?
case "$config_status" in
5) # ok, nothing to unset
@@ -213,8 +220,15 @@ test_unconfig () {
 
 # Set git config, automatically unsetting it after the test is over.
 test_config () {
-   test_when_finished "test_unconfig '$1'" &&
-   git config "$@"
+   config_dir=
+   if test "$1" = -C
+   then
+   shift
+   config_dir=$1
+   shift
+   fi
+   test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" 
&&
+   git ${config_dir:+-C "$config_dir"} config "$@"
 }
 
 test_config_global () {
-- 
2.5.0.466.g9af26fa

--
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 5/5] test-lib-functions: detect test_when_finished in subshell

[John Keeping]

test_when_finished does nothing in a subshell because the change to
test_cleanup does not affect the parent.

There is no POSIX way to detect that we are in a subshell ($$ and $PPID
are specified to remain unchanged), but we can detect it on Bash and
fall back to ignoring the bug on other shells.

Signed-off-by: John Keeping 
---
 t/test-lib-functions.sh | 5 +
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0e80f37..6dffb8b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -736,6 +736,11 @@ test_seq () {
 # what went wrong.
 
 test_when_finished () {
+   # We cannot detect when we are in a subshell in general, but by
+   # doing so on Bash is better than nothing (the test will
+   # silently pass on other shells).
+   test "${BASH_SUBSHELL-0}" = 0 ||
+   error "bug in test script: test_when_finished does nothing in a 
subshell"
test_cleanup="{ $*
} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }
-- 
2.5.0.466.g9af26fa

--
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: [L10N] Kickoff of translation for Git 2.6.0 round 1

[John Keeping]

On Sat, Sep 05, 2015 at 09:14:18PM +0800, Jiang Xin wrote:
> 2015-09-05 18:02 GMT+08:00 Jean-Noël AVILA :
> > Le samedi 5 septembre 2015, 10:17:54 Jiang Xin a écrit :
> >> Hi,
> >>
> >> Git v2.6.0-rc0 has been released, and it's time to start new round of git
> >> l10n. This time there are 123 updated messages need to be translated since
> >> last update:
> >>
> >> l10n: git.pot: v2.6.0 round 1 (123 new, 41 removed)
> >>
> >> Generate po/git.pot from v2.6.0-rc0-24-gec371ff for git v2.6.0 l10n
> >> round 1.
> >>
> >> Signed-off-by: Jiang Xin 
> >>
> >> You can get it from the usual place:
> >>
> >> https://github.com/git-l10n/git-po/
> >>
> >> As how to update your XX.po and help to translate Git, please see
> >> "Updating a XX.po file" and other sections in “po/README" file.
> >>
> >> --
> >> Jiang Xin
> >
> >
> > Some new strings are not consistent with the actual set.
> >
> > For instance, in the "Could not ..." strings were all with capitals, and 
> > some
> > new ones are not. Last time, I remarked strings which were almost exactly 
> > the
> > same (only difference was a final dot, if I remember). Some help strings 
> > were
> > mixing different styles.
> 
> Before this update, "Could not" vs "could not" is 50:40, and now it's 50:50.
> 
> $ grep 'Could not' po/git.pot  | wc -l
> 50
> 
> $ grep 'could not' po/git.pot  | wc -l
> 50

Note that Documentation/CodingGuidelines has a section on error messages
which says:

 - Do not end error messages with a full stop.

 - Do not capitalize ("unable to open %s", not "Unable to open %s")

 - Say what the error is first ("cannot open %s", not "%s: cannot open")
--
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] interpret-trailers: allow running outside a repository

[John Keeping]

It may be useful to run git-interpret-trailers without needing to be in
a repository.

Signed-off-by: John Keeping 
---
 git.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 82d7a1c..2431cb0 100644
--- a/git.c
+++ b/git.c
@@ -417,7 +417,7 @@ static struct cmd_struct commands[] = {
{ "index-pack", cmd_index_pack, RUN_SETUP_GENTLY },
{ "init", cmd_init_db, NO_SETUP },
{ "init-db", cmd_init_db, NO_SETUP },
-   { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP },
+   { "interpret-trailers", cmd_interpret_trailers, RUN_SETUP_GENTLY },
{ "log", cmd_log, RUN_SETUP },
{ "ls-files", cmd_ls_files, RUN_SETUP },
{ "ls-remote", cmd_ls_remote, RUN_SETUP_GENTLY },
-- 
2.5.0.466.g9af26fa

--
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/5] disallow test_when_finished in subshells

[John Keeping]

On Sat, Sep 05, 2015 at 10:36:29AM -0700, Junio C Hamano wrote:
> On Sat, Sep 5, 2015 at 6:12 AM, John Keeping  wrote:
> >
> > I don't think it's worth trying to clear $BASH_SUBSHELL before the tests
> > start because to do so we have to reliably detect that we're not running
> > under Bash, and if we don't trust people not to set $BASH_SUBSHELL why
> > do we trust them not to set $BASH?
> 
> I am not worried about evil people who do funny things to deliberately break
> other people's arrangement. I am more worried about stupid people (e.g. those
> who export CDPATH).
> 
> In bash a stupid person may attempt to export BASH_SUBSHELL and then
> have a script that runs our test suite, setting SHELL_PATH to point at a
> non-bash while building Git and running the tests under a non-bash shell. I
> am hesitant to believe that we will know the variable will never leak through
> to the test via environment.
> 
> Isn't it just the matter of resetting the variable regardless of $BASH
> (and ignoring
> a possible refusal to do so under bash) at the beginning of the test, or do 
> you
> really have to rely on the value of $BASH and do things differently?

Bash doesn't refuse to set it, it lets you update the value; I did think
that it wouldn't update it if the user had overridden the value, but it
looks like that was only because I had unset it first.  It seems that
the variable is magic (autoincrementing in subshells and can only be set
to integer values) but if you unset it then it becomes a normal
variable.

I'm wary of relying on that behaviour being unchanged across all
versions of bash, so I'd prefer to avoid writing the variable if running
under bash.

We do already have t/lib-bash.sh which has an optimization to avoid
exec'ing bash if "$BASH" is set, which will break in the same way if
someone exports BASH and then runs t9902 or t9903 under non-bash.
--
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 5/5] test-lib-functions: detect test_when_finished in subshell

[John Keeping]

On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:
> On Sat, Sep 5, 2015 at 9:12 AM, John Keeping  wrote:
> > test_when_finished does nothing in a subshell because the change to
> > test_cleanup does not affect the parent.
> >
> > There is no POSIX way to detect that we are in a subshell ($$ and $PPID
> > are specified to remain unchanged), but we can detect it on Bash and
> > fall back to ignoring the bug on other shells.
> 
> I'm not necessarily advocating this, but think it's worth mentioning
> that an alternate solution would be to fix test_when_finished() to work
> correctly in subshells rather than disallowing its use. This can be done
> by having test_when_finished() collect the cleanup actions in a file
> rather than in a shell variable.
> 
> Pros:
> * works in subshells
> * portable across all shells (no Bash special-case)
> * one less rule (restriction) for test writers to remember
> 
> Cons:
> * slower
> * could interfere with tests expecting very specific 'trash' directory
>   contents (but locating this file under .git might make it safe)

Another con is that we have to worry about the working directory, and
since we can't reliably detect if we're in a subshell every cleanup
action probably has to be something like:

( cd '$(pwd)' && $* )

It's certainly possible but it adds another bit of complexity.

Since there are only 3 out of over 13,000 tests that use this
functionality (and it's quite easy to change them not to) I'm not sure
it's worth supporting it.

> > Signed-off-by: John Keeping 
> > ---
> >  t/test-lib-functions.sh | 5 +
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 0e80f37..6dffb8b 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -736,6 +736,11 @@ test_seq () {
> >  # what went wrong.
> >
> >  test_when_finished () {
> > +   # We cannot detect when we are in a subshell in general, but by
> > +   # doing so on Bash is better than nothing (the test will
> > +   # silently pass on other shells).
> > +   test "${BASH_SUBSHELL-0}" = 0 ||
> > +   error "bug in test script: test_when_finished does nothing in a 
> > subshell"
> > test_cleanup="{ $*
> > } && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
> >  }
> > --
> > 2.5.0.466.g9af26fa
--
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] Documentation/git-rebase: fix --no-autostash formatting

[John Keeping]

All of the other "--option" and "--no-option" pairs in this file are
formatted as separate options.

Signed-off-by: John Keeping 
---
 Documentation/git-rebase.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ca03954..72e69fc 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -432,7 +432,8 @@ If the '--autosquash' option is enabled by default using the
 configuration variable `rebase.autoSquash`, this option can be
 used to override and disable this setting.
 
---[no-]autostash::
+--autostash::
+--no-autostash::
Automatically create a temporary stash before the operation
begins, and apply it after the operation ends.  This means
that you can run rebase on a dirty worktree.  However, use
-- 
2.6.0.rc0.162.gb2d3693

--
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] rebase: support --no-autostash

[John Keeping]

This is documented as an option but we don't actually accept it.
Support it so that it is possible to override the "rebase.autostash"
config variable.

Reported-by: Daniel Hahler 
Signed-off-by: John Keeping 
---
 git-rebase.sh   |  5 -
 t/t3420-rebase-autostash.sh | 10 ++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 1757404..af7ba5f 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -14,7 +14,7 @@ git-rebase --continue | --abort | --skip | --edit-todo
  Available options are
 v,verbose! display a diffstat of what changed upstream
 q,quiet!   be quiet. implies --no-stat
-autostash! automatically stash/stash pop before and after
+autostash  automatically stash/stash pop before and after
 fork-point use 'merge-base --fork-point' to refine upstream
 onto=! rebase onto given branch instead of upstream
 p,preserve-merges! try to recreate merges instead of ignoring them
@@ -292,6 +292,9 @@ do
--autostash)
autostash=true
;;
+   --no-autostash)
+   autostash=false
+   ;;
--verbose)
verbose=t
diffstat=t
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index d783f03..944154b 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -37,6 +37,16 @@ testrebase() {
type=$1
dotest=$2
 
+   test_expect_success "rebase$type: dirty worktree, --no-autostash" '
+   test_config rebase.autostash true &&
+   git reset --hard &&
+   git checkout -b rebased-feature-branch feature-branch &&
+   test_when_finished git branch -D rebased-feature-branch &&
+   test_when_finished git checkout feature-branch &&
+   echo dirty >>file3 &&
+   test_must_fail git rebase$type --no-autostash 
unrelated-onto-branch
+   '
+
test_expect_success "rebase$type: dirty worktree, non-conflicting 
rebase" '
test_config rebase.autostash true &&
git reset --hard &&
-- 
2.6.0.rc0.162.gb2d3693

--
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] rebase: support --no-autostash

[John Keeping]

The first patch is the fix so that git-rebase supports the
--no-autostash option that it's documentation claims it does.

The second is a slight tweak to the documentation to make it consistent
with the remainder of the file.

John Keeping (2):
  rebase: support --no-autostash
  Documentation/git-rebase: fix --no-autostash formatting

 Documentation/git-rebase.txt |  3 ++-
 git-rebase.sh|  5 -
 t/t3420-rebase-autostash.sh  | 10 ++
 3 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.6.0.rc0.162.gb2d3693

--
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 submodule ignores --git-dir

[John Keeping]

On Fri, Sep 11, 2015 at 05:15:52PM +0200, Jens Lehmann wrote:
> Am 10.09.2015 um 22:06 schrieb Filip Gospodinov:
> > I know that for this particular use case I can just use `git clone 
> > --recursive`
> > and that other use cases can be worked around by using `cd`. Still, I 
> > wonder if
> > the behavior I discovered is a bug or if it's expected.
> 
> I don't think this is a bug. The git submodule command needs a work tree
> to read the .gitmodules file from, that's while it fails when using
> --git-dir from outside the work tree. But I admit that the error message
> "No submodule mapping found in .gitmodules for path ..." could be improved
> to clearly state that the .gitmodules file wasn't found.
> 
> Unfortunately trying to show git the right work tree:
> 
> $ git --git-dir=$PWD/repo2/.git --work-tree=$PWD/repo2 submodule update --init
> 
> Didn't work as I expected it to either:
> 
> fatal: /home/Sledge/libexec/git-core/git-submodule cannot be used without a 
> working tree.
> 
> So you'll have to cd into the repo for now.

There's also "git -C /path/to/repo" which avoids the need for a separate
"cd".
--
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 v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[John Keeping]

On Thu, Sep 10, 2015 at 09:52:49PM +0530, Karthik Nayak wrote:
> Use 'ref-filter' APIs to implement the '--merged' and '--no-merged'
> options into 'tag.c'. The '--merged' option lets the user to only list
> tags merged into the named commit. The '--no-merged' option lets the
> user to only list tags not merged into the named commit.  If no object
> is provided it assumes HEAD as the object.
> 
> Add documentation and tests for the same.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Matthieu Moy 
> Signed-off-by: Karthik Nayak 
> ---
>  Documentation/git-tag.txt |  7 ++-
>  builtin/tag.c |  6 +-
>  t/t7004-tag.sh| 27 +++
>  3 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 0c7f4e6..3803bf7 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -14,7 +14,7 @@ SYNOPSIS
>  'git tag' -d ...
>  'git tag' [-n[]] -l [--contains ] [--points-at ]
>   [--column[=] | --no-column] [--create-reflog] [--sort=]
> - [--format=] [...]
> + [--format=] [--[no-]merged []] [...]
>  'git tag' -v ...
>  
>  DESCRIPTION
> @@ -165,6 +165,11 @@ This option is only applicable when listing tags without 
> annotation lines.
>   that of linkgit:git-for-each-ref[1].  When unspecified,
>   defaults to `%(refname:short)`.
>  
> +--[no-]merged []::

We prefer to write --[no-]* as:

--option::
--no-option::

although this may be the first instance that we see this combination
with an argument.

I also found the "[]" syntax confusing and had to go and figure
out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
appending something like the following to the help for this option:

The `commit` may be omitted if this is the final argument.

> + Only list tags whose tips are reachable, or not reachable
> + if '--no-merged' is used, from the specified commit ('HEAD'
> + if not specified).
> +
>  CONFIGURATION
>  -
>  By default, 'git tag' in sign-with-default mode (-s) will use your
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 075d90b..081fe84 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -23,7 +23,7 @@ static const char * const git_tag_usage[] = {
>   N_("git tag [-a | -s | -u ] [-f] [-m  | -F ] 
>  []"),
>   N_("git tag -d ..."),
>   N_("git tag -l [-n[]] [--contains ] [--points-at ]"
> - "\n\t\t[--format=] [...]"),
> + "\n\t\t[--format=] [--[no-]merged []] 
> [...]"),
>   N_("git tag -v ..."),
>   NULL
>  };
> @@ -359,6 +359,8 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   OPT_COLUMN(0, "column", &colopts, N_("show tag list in 
> columns")),
>   OPT_CONTAINS(&filter.with_commit, N_("print only tags that 
> contain the commit")),
>   OPT_WITH(&filter.with_commit, N_("print only tags that contain 
> the commit")),
> + OPT_MERGED(&filter, N_("print only tags that are merged")),
> + OPT_NO_MERGED(&filter, N_("print only tags that are not 
> merged")),
>   OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"),
>N_("field name to sort on"), 
> &parse_opt_ref_sorting),
>   {
> @@ -417,6 +419,8 @@ int cmd_tag(int argc, const char **argv, const char 
> *prefix)
>   die(_("--contains option is only allowed with -l."));
>   if (filter.points_at.nr)
>   die(_("--points-at option is only allowed with -l."));
> + if (filter.merge_commit)
> + die(_("--merged and --no-merged option are only allowed with 
> -l"));
>   if (cmdmode == 'd')
>   return for_each_tag_name(argv, delete_tag);
>   if (cmdmode == 'v')
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 8987fb1..3dd2f51 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1531,4 +1531,31 @@ test_expect_success '--format should list tags as per 
> format given' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'setup --merged test tags' '
> + git tag mergetest-1 HEAD~2 &&
> + git tag mergetest-2 HEAD~1 &&
> + git tag mergetest-3 HEAD
> +'
> +
> +test_expect_success '--merged cannot be used in non-list mode' '
> + test_must_fail git tag --merged=mergetest-2 foo
> +'
> +
> +test_expect_success '--merged shows merged tags' '
> + cat >expect <<-\EOF &&
> + mergetest-1
> + mergetest-2
> + EOF
> + git tag -l --merged=mergetest-2 mergetest-* >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--no-merged show unmerged tags' '
> + cat >expect <<-\EOF &&
> + mergetest-3
> + EOF
> + git tag -l --no-merged=mergetest-2 mergetest-* >actual &&
> + test_cmp expect actual
> +'
> +
>  test_done
> -- 
> 2.5.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kerne

Re: [PATCH v17 14/14] tag.c: implement '--merged' and '--no-merged' options

[John Keeping]

On Fri, Sep 18, 2015 at 09:10:08AM +0200, Matthieu Moy wrote:
> Junio C Hamano  writes:
> 
> > John Keeping  writes:
> >
> >>> +--[no-]merged []::
> >>
> >> We prefer to write --[no-]* as:
> >>
> >>--option::
> >>--no-option::
> >>
> >> although this may be the first instance that we see this combination
> >> with an argument.
> >>
> >> I also found the "[]" syntax confusing and had to go and figure
> >> out what PARSE_OPT_LASTARG_DEFAULT does; I wonder if it's worth
> >> appending something like the following to the help for this option:
> >>
> >>The `commit` may be omitted if this is the final argument.
> >
> > "may be omitted" must be followed by a description of what happens
> > when omitted (i.e. "defaults to ...").
> 
> Then:
> 
> The `commit` may be omitted and defaults to HEAD if this is the final
> argument.

I find that slightly confusing, although that might just be me.  It's
slightly longer, but I would write:

The `commit` may be omitted if this is the final argument, in
which case it defaults to `HEAD`.

I also had a look at git-branch(1), which has similar `--merged` and
`--no-merged` options and says:

Only list branches whose tips are reachable from the specified
commit (HEAD if not specified).  Implies `--list`.

The two options are listed separately in that case.
--
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: Formatting error in page http://git-scm.com/docs/user-manual

[John Keeping]

On Fri, Sep 25, 2015 at 03:38:02PM +0300, Valentin VALCIU wrote:
> There is a formatting error in the source code of page
> http://git-scm.com/docs/user-manual that makes almost half of it be
> rendered in a  element displaying the page source in the original
> markup language instead of being converted to HTML.
> 
> The issue is in the paragraph that stars with "The index is a binary
> file (generally kept in `.git/index`)..."

It looks like the header underline isn't quite accurate enough to keep
Asciidoctor happy.  The patch below should fix it.

-- >8 --
Subject: [PATCH] Documentation/user-manual: fix header underline

Asciidoctor is stricter than AsciiDoc when deciding if underlining is a
section title or the start of preformatted text.  Make the length of the
underlining match the text to ensure that it renders correctly in all
implementations.

Signed-off-by: John Keeping 
---
 Documentation/user-manual.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 68978f5..1b7987e 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -3424,7 +3424,7 @@ just missing one particular blob version.
 
 [[the-index]]
 The index

+-
 
 The index is a binary file (generally kept in `.git/index`) containing a
 sorted list of path names, each with permissions and the SHA-1 of a blob
-- 
2.6.0.rc2.198.g81437b7
--
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 ?

[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

Re: [BUG?] parallel make interdepencies

[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: Using clean/smudge filters with difftool

[John Keeping]

[Please don't top-post on this list.]

On Thu, Jun 18, 2015 at 03:15:38PM +0200, Florian Aspart wrote:
> 2015-06-18 14:31 GMT+02:00 Michael J Gruber :
> > Florian Aspart venit, vidit, dixit 16.06.2015 16:11:
> >> Hi everyone,
> >>
> >> I created a clean filter to apply on some files before commiting them.
> >> The filter works correctly when I commit the file and is also applied
> >> when I usethe iff command line tool.
> >> However, when using difftool with meld, the filter is not applied and
> >> the different versions of the files are compared without any
> >> filtering.
> >>
> >> Is there a way to apply the clean/smudge filters when comparing the
> >> working copy of a file to the HEAD version in a gui diff tool?
> >>
> >> I'm using git version 2.4.3 under Ubuntu.
> >>
> >> Best,
> >> Florian
> >
> > Are you saying that "difftool" compares an uncleaned working tree file
> > with a cleaned blob? That would be a bug in either difftool or the way
> > we feed difftool.
> >
> yes in this case "difftool" compares an uncleaned working tree file
> with a cleaned blob. I did not try the smudge filter to see if it
> applied in difftool.
> 
> I think the problem comes from the way difftool is feeded, since I
> also had this problem when setting an external tool for the diff in
> the gitconfig file.
> 
> However, I'm not sure if this is a bug or it is designed to be so.
> If the external tool changes a cleaned working tree file during the
> diff, then by saving this file the result of the cleaning filter would
> also be saved in the working tree.

How is your filter configured?  Is it using a simple pattern (e.g.
"*.c") or is it using a file path?

git-difftool uses `git checkout-index --all --prefix=$dir/` and I wonder
if the prefix means that the attribute specification does not match the
temporary file that difftool produces, so no filter is applied.
--
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: Using clean/smudge filters with difftool

[John Keeping]

On Thu, Jun 18, 2015 at 03:51:25PM +0200, Florian Aspart wrote:
> 2015-06-18 15:26 GMT+02:00 John Keeping :
> > [Please don't top-post on this list.]
> >
> > On Thu, Jun 18, 2015 at 03:15:38PM +0200, Florian Aspart wrote:
> >> 2015-06-18 14:31 GMT+02:00 Michael J Gruber :
> >> > Florian Aspart venit, vidit, dixit 16.06.2015 16:11:
> >> >> Hi everyone,
> >> >>
> >> >> I created a clean filter to apply on some files before commiting them.
> >> >> The filter works correctly when I commit the file and is also applied
> >> >> when I usethe iff command line tool.
> >> >> However, when using difftool with meld, the filter is not applied and
> >> >> the different versions of the files are compared without any
> >> >> filtering.
> >> >>
> >> >> Is there a way to apply the clean/smudge filters when comparing the
> >> >> working copy of a file to the HEAD version in a gui diff tool?
> >> >>
> >> >> I'm using git version 2.4.3 under Ubuntu.
> >> >>
> >> >> Best,
> >> >> Florian
> >> >
> >> > Are you saying that "difftool" compares an uncleaned working tree file
> >> > with a cleaned blob? That would be a bug in either difftool or the way
> >> > we feed difftool.
> >> >
> >> yes in this case "difftool" compares an uncleaned working tree file
> >> with a cleaned blob. I did not try the smudge filter to see if it
> >> applied in difftool.
> >>
> >> I think the problem comes from the way difftool is feeded, since I
> >> also had this problem when setting an external tool for the diff in
> >> the gitconfig file.
> >>
> >> However, I'm not sure if this is a bug or it is designed to be so.
> >> If the external tool changes a cleaned working tree file during the
> >> diff, then by saving this file the result of the cleaning filter would
> >> also be saved in the working tree.
> >
> > How is your filter configured?  Is it using a simple pattern (e.g.
> > "*.c") or is it using a file path?
> >
> > git-difftool uses `git checkout-index --all --prefix=$dir/` and I wonder
> > if the prefix means that the attribute specification does not match the
> > temporary file that difftool produces, so no filter is applied.
> 
> It is using a simple pattern:
> *.ipynb filter=clean_ipynb

I also realised that the code for file diff is very different from
directory diff do you see any difference between git-difftool acting on
files and with the `--dir-diff` option?
--
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: Using clean/smudge filters with difftool

[John Keeping]

On Thu, Jun 18, 2015 at 04:17:52PM +0200, Florian Aspart wrote:
> 2015-06-18 16:11 GMT+02:00 John Keeping :
> > On Thu, Jun 18, 2015 at 03:51:25PM +0200, Florian Aspart wrote:
> >> 2015-06-18 15:26 GMT+02:00 John Keeping :
> >> > [Please don't top-post on this list.]
> >> >
> >> > On Thu, Jun 18, 2015 at 03:15:38PM +0200, Florian Aspart wrote:
> >> >> 2015-06-18 14:31 GMT+02:00 Michael J Gruber :
> >> >> > Florian Aspart venit, vidit, dixit 16.06.2015 16:11:
> >> >> >> I created a clean filter to apply on some files before commiting 
> >> >> >> them.
> >> >> >> The filter works correctly when I commit the file and is also applied
> >> >> >> when I usethe iff command line tool.
> >> >> >> However, when using difftool with meld, the filter is not applied and
> >> >> >> the different versions of the files are compared without any
> >> >> >> filtering.
> >> >> >>
> >> >> >> Is there a way to apply the clean/smudge filters when comparing the
> >> >> >> working copy of a file to the HEAD version in a gui diff tool?
> >> >> >>
> >> >> >> I'm using git version 2.4.3 under Ubuntu.
> > 
> > I also realised that the code for file diff is very different from
> > directory diff do you see any difference between git-difftool acting on
> > files and with the `--dir-diff` option?
> 
> No, even with the --dir-diff option, the filter is still not applied.

I have tried to reproduce this and it works as expected for me (i.e. the
filter is applied) both for file diff and directory diff mode:

$ git config filter.quote.clean "sed -e 's/^> //'"
$ git config filter.quote.smudge "sed -e '/^> /n; s/^/> /'"
$ git config filter.quote.required true

$ echo '*.quote filter=quote' >>.gitattributes
$ cat >1.quote <>1.quote

Now `git-difftool` shows the differences with the filter applied.  This can be
seen running with GIT_TRACE:

$ GIT_TRACE=2 git difftool
15:26:59.211541 git.c:557   trace: exec: 'git-difftool'
15:26:59.211674 run-command.c:347   trace: run_command: 'git-difftool'
15:26:59.338617 git.c:348   trace: built-in: git 'config' '--bool' 
'--get' 'difftool.trustExitCode'
15:26:59.342664 git.c:348   trace: built-in: git 'diff'
15:26:59.344857 run-command.c:347   trace: run_command: 'sed -e '\''s/^> 
//'\'''
15:26:59.345383 run-command.c:195   trace: exec: '/bin/sh' '-c' 'sed -e 
'\''s/^> //'\''' 'sed -e '\''s/^> //'\'''
15:26:59.351077 run-command.c:347   trace: run_command: 'sed -e '\''/^> /n; 
s/^/> /'\'''
15:26:59.351605 run-command.c:195   trace: exec: '/bin/sh' '-c' 'sed -e 
'\''/^> /n; s/^/> /'\''' 'sed -e '\''/^> /n; s/^/> /'\'''
15:26:59.355716 run-command.c:347   trace: run_command: 
'git-difftool--helper' '1.quote' '/tmp/SUEySx_1.quote' 
'4cb29ea38f70d7c61b2a3a25b02e3bdf44905402' '100644' '1.quote' 
'' '100644'
15:26:59.356191 run-command.c:195   trace: exec: 'git-difftool--helper' 
'1.quote' '/tmp/SUEySx_1.quote' '4cb29ea38f70d7c61b2a3a25b02e3bdf44905402' 
'100644' '1.quote' '' '100644'
15:26:59.370468 git.c:348   trace: built-in: git 'config' 
'diff.tool'
15:26:59.373485 git.c:348   trace: built-in: git 'config' 
'merge.tool'
15:26:59.378402 git.c:348   trace: built-in: git 'config' 
'difftool.vimdiff.cmd'
15:26:59.381424 git.c:348   trace: built-in: git 'config' 
'mergetool.vimdiff.cmd'
15:26:59.386623 git.c:348   trace: built-in: git 'config' '--bool' 
'mergetool.prompt'
15:26:59.390198 git.c:348   trace: built-in: git 'config' '--bool' 
'difftool.prompt'

I think the first run_command of `sed` is cleaning the working tree file
to figure out *if* it differs, then the second `sed` is smudging the
version in the index so that difftool can use it.
--
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: Using clean/smudge filters with difftool

[John Keeping]

On Thu, Jun 18, 2015 at 05:39:18PM +0200, Florian Aspart wrote:
> 2015-06-18 16:28 GMT+02:00 John Keeping :
> > On Thu, Jun 18, 2015 at 04:17:52PM +0200, Florian Aspart wrote:
> >> 2015-06-18 16:11 GMT+02:00 John Keeping :
> >> > On Thu, Jun 18, 2015 at 03:51:25PM +0200, Florian Aspart wrote:
> >> >> 2015-06-18 15:26 GMT+02:00 John Keeping :
> >> >> > [Please don't top-post on this list.]
> >> >> >
> >> >> > On Thu, Jun 18, 2015 at 03:15:38PM +0200, Florian Aspart wrote:
> >> >> >> 2015-06-18 14:31 GMT+02:00 Michael J Gruber 
> >> >> >> :
> >> >> >> > Florian Aspart venit, vidit, dixit 16.06.2015 16:11:
> >> >> >> >> I created a clean filter to apply on some files before commiting 
> >> >> >> >> them.
> >> >> >> >> The filter works correctly when I commit the file and is also 
> >> >> >> >> applied
> >> >> >> >> when I usethe iff command line tool.
> >> >> >> >> However, when using difftool with meld, the filter is not applied 
> >> >> >> >> and
> >> >> >> >> the different versions of the files are compared without any
> >> >> >> >> filtering.
> >> >> >> >>
> >> >> >> >> Is there a way to apply the clean/smudge filters when comparing 
> >> >> >> >> the
> >> >> >> >> working copy of a file to the HEAD version in a gui diff tool?
> >> >> >> >>
> >> >> >> >> I'm using git version 2.4.3 under Ubuntu.
> >> >
> >> > I also realised that the code for file diff is very different from
> >> > directory diff do you see any difference between git-difftool acting on
> >> > files and with the `--dir-diff` option?
> >>
> >> No, even with the --dir-diff option, the filter is still not applied.
> >
> > I have tried to reproduce this and it works as expected for me (i.e. the
> > filter is applied) both for file diff and directory diff mode:
> >
> > $ git config filter.quote.clean "sed -e 's/^> //'"
> > $ git config filter.quote.smudge "sed -e '/^> /n; s/^/> /'"
> > $ git config filter.quote.required true
> >
> > $ echo '*.quote filter=quote' >>.gitattributes
> > $ cat >1.quote < > one
> > two
> > three
> > EOF
> > $ git add .gitattributes 1.quote
> > $ git commit -m 'Initial commit'
> > $ echo four >>1.quote
> >
> > Now `git-difftool` shows the differences with the filter applied.  This can 
> > be
> > seen running with GIT_TRACE:
> >
> > $ GIT_TRACE=2 git difftool
> > 15:26:59.211541 git.c:557   trace: exec: 'git-difftool'
> > 15:26:59.211674 run-command.c:347   trace: run_command: 'git-difftool'
> > 15:26:59.338617 git.c:348   trace: built-in: git 'config' 
> > '--bool' '--get' 'difftool.trustExitCode'
> > 15:26:59.342664 git.c:348   trace: built-in: git 'diff'
> > 15:26:59.344857 run-command.c:347   trace: run_command: 'sed -e 
> > '\''s/^> //'\'''
> > 15:26:59.345383 run-command.c:195   trace: exec: '/bin/sh' '-c' 'sed -e 
> > '\''s/^> //'\''' 'sed -e '\''s/^> //'\'''
> > 15:26:59.351077 run-command.c:347   trace: run_command: 'sed -e '\''/^> 
> > /n; s/^/> /'\'''
> > 15:26:59.351605 run-command.c:195   trace: exec: '/bin/sh' '-c' 'sed -e 
> > '\''/^> /n; s/^/> /'\''' 'sed -e '\''/^> /n; s/^/> /'\'''
> > 15:26:59.355716 run-command.c:347   trace: run_command: 
> > 'git-difftool--helper' '1.quote' '/tmp/SUEySx_1.quote' 
> > '4cb29ea38f70d7c61b2a3a25b02e3bdf44905402' '100644' '1.quote' 
> > '' '100644'
> > 15:26:59.356191 run-command.c:195   trace: exec: 'git-difftool--helper' 
> > '1.quote' '/tmp/SUEySx_1.quote' '4cb29ea38f70d7c61b2a3a25b02e3bdf44905402' 
>

Re: Using clean/smudge filters with difftool

[John Keeping]

On Thu, Jun 18, 2015 at 01:00:36PM -0700, Junio C Hamano wrote:
> John Keeping  writes:
> 
> > I think this is a difference between git-diff's internal and external
> > diff modes which is working correctly, although possibly not desirably
> > in this case.  The internal diff always uses clean files (so it runs the
> > working tree file through the "clean" filter before applying the diff
> > algorithm) but the external diff uses the working tree file so it
> > applies the "smudge" filter to any blobs that it needs to checkout.
> >
> > Commit 4e218f5 (Smudge the files fed to external diff and textconv,
> > 2009-03-21) was the source of this behaviour.
> 
> The fundamental design to use smudged version when interacting with
> external programs actually predates that particular commit, I think.
> 
> The caller of the function that was updated by that commit, i.e.
> prepare_temp_file(), reuses what is checked out to the working tree
> when we can (i.e. it hasn't been modified from what we think is
> checked out) and when it is beneficial to do so (i.e. on a system
> with FAST_WORKING_DIRECTORY defined), which means the temporary file
> given by the prepare_temp_file() that is used by the external tools
> (both --ext-diff program and textconv filter) are designed to be fed
> and work on the smudged version of the file.  4e218f5 did not change
> that fundamental design; it just made things more consistent between
> the case where we do create a new temporary file out of blob and we
> allow an unmodified checked out file to be reused.

When I started looking at this, I assumed the problem would be that
git-difftool wasn't smudging the non-working-tree files.  But actually
everything is working "correctly", I'm just not sure it's always what
the user wants (at least it isn't what was wanted in this case).

Currently, the behaviour is:

internal diff: compare clean files
external diff: compare smudged files

This makes sense for LF/CRLF conversion, where platform-specific tools
clearly want the platform's line ending but the internal diff machinery
doesn't care.

However, from the filter description in an earlier email, I think
Florian is using a clean filter to remove output from IPython notebook
files (it seems that IPython saves both the input and the output in the
same file [1] and the output is the equivalent of, for example, C object
files).  In this case, the filter is one-way and discards information
from the working tree file, producing a smaller and more readable diff
in the process.

I think the summary is that there are some scenarios where the external
diff tool should see the smudged version and others where the clean
version is more appropriate and Git should support both options.  It
seems this is a property of the filter, so I wonder if the best solution
is a new "filter..extdiff = [clean|smudge]" configuration
variable (there's probably a better name for the variable than
"extdiff").


[1] http://pascalbugnion.net/blog/ipython-notebooks-and-git.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: Using clean/smudge filters with difftool

[John Keeping]

On Fri, Jun 19, 2015 at 10:57:55AM +0200, Michael J Gruber wrote:
> Junio C Hamano venit, vidit, dixit 19.06.2015 00:55:
> > John Keeping  writes:
> > 
> >> I think the summary is that there are some scenarios where the external
> >> diff tool should see the smudged version and others where the clean
> >> version is more appropriate and Git should support both options.  It
> >> seems this is a property of the filter, so I wonder if the best solution
> >> is a new "filter..extdiff = [clean|smudge]" configuration
> >> variable (there's probably a better name for the variable than
> >> "extdiff").
> > 
> > Not just the external diff, but the textconv filter obeys the same
> > rule.  The setting should be done the same way for both, if we are
> > going to go in that direction.
> > 
> 
> textconv is a "one-way" filter from "blob" to "readable blob". External
> diffs may prefer to work on "blob" rather than "readable blob", but the
> currect setup does not seem to produce surprises.
> 
> clean and smudge are two-way filters: clean from "worktree blob" (aka
> file) to "repo blob", smudge the other way round.
> 
> Typically, the user perceives these as inverse to each other. But we
> only require clean to be a left-inverse of smudge, i.e. "(cat-file then)
> smudge then clean" should give the same "repo blob" (as "cat-file").
> 
> We don't require that the other way round, i.e. we don't require smudge
> to be a left-inverse of clean, and in most setups (like the current one)
> it is not: smudge does not recreate what clean has cleaned out. It is a
> no-op (the "identity", while clean is a "projection").
> 
> Now, since external diff runs on smudged blobs, it appears as if we
> mixed cleaned and smudged blobs when feeding external diffs; whereas
> really, we mix "worktree blobs" and "smudged repo blobs", which is okay
> as per our definition of clean/smudge: the difference is irrelevant by
> definition.

I agree with this.

But I was wrong that "should diff clean"/"should diff smudged" is a
property of the filter.  I can also imagine a situation where a more
intelligent external diff tool wants to see the smudged version where a
naïve tool would want the clean version.

For example, some of the big file stores (e.g. git-lfs [1]) use
clean/smudge filters and I can imagine a diff utility that avoids
needing to fetch the data for large files and instead shows the diff on
the server when both blobs are available there.  In that case we
generally want to use the smudged copy for external diff, so the filter
would use that setting, but the diff utility knows better and would want
to override that.

[1] https://github.com/github/git-lfs
--
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 3/3] Add filter-objects command

[John Keeping]

On Fri, Jun 19, 2015 at 11:33:24AM +0100, Charles Bailey wrote:
> So, yes, performance is definitely an issue and I could have called this
> command "git magically-generate-all-object-for-scripts" but then, as it
> was so easy to provide exactly the filtering that I was looking for in
> the C code, I thought I would do that as well and then "filter-objects"
> ("filter-all-objects"?) seemed like a better name.

By analogy with "git filter-branch", I don't think "filter-objects" is a
good name here.  My preference would be "ls-objects".
--
To unsubscribe from this list: send the line "unsubscribe git" in

Re: [PATCH 2/3] convert "enum date_mode" into a struct

[John Keeping]

On Thu, Jun 25, 2015 at 12:55:02PM -0400, Jeff King wrote:
> In preparation for adding date modes that may carry extra
> information beyond the mode itself, this patch converts the
> date_mode enum into a struct.
> 
> Most of the conversion is fairly straightforward; we pass
> the struct as a pointer and dereference the type field where
> necessary. Locations that declare a date_mode can use a "{}"
> constructor.  However, the tricky case is where we use the
> enum labels as constants, like:
> 
>   show_date(t, tz, DATE_NORMAL);
> 
> Ideally we could say:
> 
>   show_date(t, tz, &{ DATE_NORMAL });
> 
> but of course C does not allow that.

Yes it does, e.g. in 6.5.2.5 of C11, example 3 shows:

drawline(&(struct point){.x=1, .y=1},
&(struct point){.x=3, .y=4});

The cast is required, but if the argument is pointer-to-const you can
construct a temporary in the function call.

Of course, whether all of the compilers we target support it is a
different question.  If they do, perhaps something like:

#define SIMPLE_DATE(f)  &(struct date_mode) { DATE_NORMAL }

would allow the callers to remain reasonably sane.

>  Likewise, we cannot
> cast the constant to a struct, because we need to pass an
> actual address. Our options are basically:
> 
>   1. Manually add a "struct date_mode d = { DATE_NORMAL }"
>  definition to each caller, and pass "&d". This makes
>  the callers uglier, because they sometimes do not even
>  have their own scope (e.g., they are inside a switch
>  statement).
> 
>   2. Provide a pre-made global "date_normal" struct that can
>  be passed by address. We'd also need "date_rfc2822",
>  "date_iso8601", and so forth. But at least the ugliness
>  is defined in one place.
> 
>   3. Provide a wrapper that generates the correct struct on
>  the fly. The big downside is that we end up pointing to
>  a single global, which makes our wrapper non-reentrant.
>  But show_date is already not reentrant, so it does not
>  matter.
> 
> This patch implements 3, along with a minor macro to keep
> the size of the callers sane.
--
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] --count feature for git shortlog

[John Keeping]

On Tue, Jun 30, 2015 at 02:10:49PM +0200, Johannes Schindelin wrote:
> On 2015-06-29 18:46, Lawrence Siebert wrote:
> 
> > I appreciate your help. Okay, That all makes sense.
> > 
> > I would note that something like:
> >  git shortlog -s "$FILENAME:  | cut -f 1 | paste -sd+ - | bc
> > 
> > seems like it run much faster then:
> > 
> >  git log --oneline "$FILENAME" | wc -l
> 
> How does it compare to `git rev-list -- "$FILENAME" | wc -l`?

Or even `git rev-list --count HEAD -- "$FILENAME"`.
--
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: undocumented core.sharedRepository=2 set by git init --shared=world

[John Keeping]

On Tue, Jul 07, 2015 at 10:14:28AM +0200, Johannes Schindelin wrote:
> Hi Joey,
> 
> On 2015-07-06 21:25, Joey Hess wrote:
> > joey@darkstar:~/tmp>git init --shared=world testrepo
> > Initialized empty shared Git repository in /home/joey/tmp/testrepo/.git/
> > joey@darkstar:~/tmp>grep shared testrepo/.git/config 
> > sharedrepository = 2
> > 
> > This magic value of 2 seems to be undocumented, as is the magic value of 1
> > that's equvilant to "group".
> > 
> > I think it would be better to have git init put in "world" or "group" and 
> > not
> > these magic values. Anyway, I suppose they ought to be documented too.
> 
> The rationale can be found here:
> https://github.com/git/git/blob/v2.4.5/builtin/init-db.c#L413-L418
> 
>   /* We do not spell "group" and such, so that
>* the configuration can be read by older version
>* of git. Note, we use octal numbers for new share modes,
>* and compatibility values for PERM_GROUP and
>* PERM_EVERYBODY.
>*/
> 
> I am sympathetic to your wish, of course, and I am sure that you
> understand why we cannot simply break other people's setups to satisfy
> it.

That comment was added in 94df250 (shared repository: optionally allow
reading to "others"., 2006-06-09) which was in 1.4.1.  I suspect that is
now sufficiently old that it no longer matters.
--
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: undocumented core.sharedRepository=2 set by git init --shared=world

[John Keeping]

On Tue, Jul 07, 2015 at 01:32:13PM +0200, Johannes Schindelin wrote:
> Hi John,
> 
> On 2015-07-07 11:10, John Keeping wrote:
> > On Tue, Jul 07, 2015 at 10:14:28AM +0200, Johannes Schindelin wrote:
> >> Hi Joey,
> >>
> >> On 2015-07-06 21:25, Joey Hess wrote:
> >> > joey@darkstar:~/tmp>git init --shared=world testrepo
> >> > Initialized empty shared Git repository in /home/joey/tmp/testrepo/.git/
> >> > joey@darkstar:~/tmp>grep shared testrepo/.git/config
> >> >  sharedrepository = 2
> >> >
> >> > This magic value of 2 seems to be undocumented, as is the magic value of 
> >> > 1
> >> > that's equvilant to "group".
> >> >
> >> > I think it would be better to have git init put in "world" or "group" 
> >> > and not
> >> > these magic values. Anyway, I suppose they ought to be documented too.
> >>
> >> The rationale can be found here:
> >> https://github.com/git/git/blob/v2.4.5/builtin/init-db.c#L413-L418
> >>
> >>/* We do not spell "group" and such, so that
> >> * the configuration can be read by older version
> >> * of git. Note, we use octal numbers for new share modes,
> >> * and compatibility values for PERM_GROUP and
> >> * PERM_EVERYBODY.
> >> */
> >>
> >> I am sympathetic to your wish, of course, and I am sure that you
> >> understand why we cannot simply break other people's setups to satisfy
> >> it.
> > 
> > That comment was added in 94df250 (shared repository: optionally allow
> > reading to "others"., 2006-06-09) which was in 1.4.1.  I suspect that is
> > now sufficiently old that it no longer matters.
> 
> I understand your point of view. With my maintainer hat on I have to
> say, though, that things like that require a major version change.
> Users tend to appreciate such a careful maintenance.

However, there has been a major version since the new syntax was
introduced (in the same commit mentioned above), so this only affects
users who initialize a repository with (say) 2.6.0 or later and then try
to use 1.4.0 or earlier to operate on it.

That means using two versions of Git released more than 9 years apart to
operate on the same repository.  IMHO even careful maintenance can
declare that an unsupported configuration.
--
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: Add '--edit-todo' to rebase

[John Keeping]

On Mon, Jul 13, 2015 at 01:27:56PM +0200, Thomas Braun wrote:
> Signed-off-by: Thomas Braun 
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index c97c648..2567a61 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1668,7 +1668,7 @@ _git_rebase ()
>  {
>   local dir="$(__gitdir)"
>   if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
> - __gitcomp "--continue --skip --abort"
> + __gitcomp "--continue --skip --abort --edit-todo"

git-rebase.sh contains:

if test "$action" = "edit-todo" && test "$type" != "interactive"
then
die "$(gettext "The --edit-todo action can only be used during 
interactive rebase.")"
fi

I wonder if it's worth doing a similar check here, which presumably
means testing if "$dir"/interactive exists.

>   return
>   fi
>   __git_complete_strategy && return
> 
> 
--
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 log fails to show all changes for a file

[John Keeping]

On Tue, Jul 14, 2015 at 09:30:35AM +0200, Olaf Hering wrote:
> 
> I wonder why this command fails to show all commits that modify a given
> function: 
> 
>   linux.git $ git log -p -M --stat -- drivers/hv/channel_mgmt.c
> 
> With commit 1f656ff3fdddc2f59649cc84b633b799908f1f7b init_vp_index() has
> "const uuid_le *type_guid" already. And somewhere between commit
> d3ba720dd58cdf6630fee4b89482c465d5ad0d0f and the one above the "const"
> was added. But git log does not show this commit.
> 
> Why is that so, and whats the command to really show all and every change?

It was added in an evil merge (f9da455b93f6ba076935b4ef4589f61e529ae046),
try:

git log -p -M --stat --cc -- drivers/hv/channel_mgmt.c
--
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: unexplained behavior/issue with git archive?

[John Keeping]

On Thu, Jul 23, 2015 at 05:23:49PM +0200, Jan Vales wrote:
> i seem to trigger behavior i do not understand with git archive.
> 
> I have this little 3 liner (vmdiff.sh):
> #!/bin/bash
> git diff --name-status "$2" "$3" > "$1.files"
> git diff --name-only "$2" "$3" |xargs -d'\n' git archive -o "$1" "$3" --
> 
> 
> For testing purpose, lets assume this call:
> # ./vmdiff.sh latest.zip HEAD^1 HEAD
> 
> # cat latest.zip.files | wc -l
> 149021
> 
> # cat latest.zip.files | egrep "^D" | wc -l
> 159
> 
> # mkdir empty; cd empty; unzip latest.zip ; find * | wc -l
> 1090
> 
> My goal is to basically diff (parts of) filesystems against each other
> and create an archive with all changed files + a file list to know what
> files were deleted. (I currently do not care about the files
> permissions+ownership, and it doesnt really matter in the current
> problem. Also dont ask, why one would store a root-filesystem in git :)
> 
> What I do not understand: why does the zip file only contains 1090
> files+dirs if the wc -l shows like 150k files and only like 159 were
> deleted?
> There should be like 149k files in that archive.
> 
> Also only the few files are all from "var" and none from etc or srv
> where definitely files changed in too! (and show up in latest.zip.files)
> 
> Is there a limit of files git archive can process?

Not explicitly, but there is a limit on the size of command lines and
xargs will invoke the command multiple times if enough arguments are
given.

What happens if you do:

git diff --name-only HEAD^ HEAD | xargs -d'\n' echo | wc -l

?

With a small number of items, there should only be one output line, but
if xargs invokes the command multiple times there will be multiple
lines.  For example (using -L2 to force a maximum of two arguments per
invocation):

$ printf '%s\n' a b c | xargs -d'\n' echo | wc -l
1
$ printf '%s\n' a b c | xargs -d'\n' -L2 echo | wc -l
2
--
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

[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 
---
 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

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

[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 
---
 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(&ref->old_oid, &ref->old_oid_expect))
+   if (oidcmp(&ref->old_oid, &ref->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, 
&ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(&ref->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, &ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(&ref->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 v2 3/3] push: allow pushing new branches with --force-with-lease

[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 
---
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(&ref->old_oid, &ref->old_oid_expect))
+   if (oidcmp(&ref->old_oid, &ref->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, 
&ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(&ref->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, &ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   memset(&ref->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 v2 1/3] Documentation/git-push: fix placeholder formatting

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping 
---
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

[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 
---
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 0/3] push: allow pushing new branches with --force-with-lease

[John Keeping]

On Mon, Jul 25, 2016 at 10:28:01AM -0700, Junio C Hamano wrote:
> John Keeping  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

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

[John Keeping]

On Mon, Jul 25, 2016 at 03:22:48PM -0700, Junio C Hamano wrote:
> John Keeping  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 
> > ---
> > 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

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

[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

[John Keeping]

On Tue, Jul 26, 2016 at 12:59:04PM -0700, Junio C Hamano wrote:
> John Keeping  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, 
> &ref->old_oid_expect))
> - memset(&ref->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, &ref->old_oid_expect))
> - memset(&ref->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

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

[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 
---
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

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

[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

[John Keeping]

Format the placeholder as monospace to match other occurrences in this
file and obey CodingGuidelines.

Signed-off-by: John Keeping 
---
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

[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 
---
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(&ref->old_oid, &ref->old_oid_expect))
+   if (oidcmp(&ref->old_oid, &ref->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, 
&ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(&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, &ref->old_oid_expect))
-   ref->expect_old_no_trackback = 1;
+   oidclr(&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 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

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

[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

Re: storing cover letter of a patch series?

[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: Is there a way to have local changes in a branch 'bake' while working in different branches?

[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

Force-with-lease and new branches

[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: [BUG] git-submodule has bash-ism?

[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

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

[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?

[John Keeping]

On Wed, Jun 01, 2016 at 12:45:11PM -0700, Junio C Hamano wrote:
> John Keeping  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: git difftool and git mergetool aren't returning errors when the tool has issues

[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

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

[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 
---
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-mergetool reverse file ordering

[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

Re: storing cover letter of a patch series?

[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: [PATCH] difftool: always honor "command not found" exit code

[John Keeping]

On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> "Tom Tanner (BLOOMBERG/ LONDON)"  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  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

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

[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 
---
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)"  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  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 v2] help: make option --help open man pages only for Git commands

[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

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

[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", &swapped, "mark as being called by  
> --help"),

OPT_HIDDEN_BOOL maybe?

>   OPT_BOOL('a', "all", &show_all, N_("print all available commands")),
>   OPT_BOOL('g', "guides", &show_guides, N_("print list of useful 
> guides")),
>   OPT_SET_INT('m', "man", &help_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: Re: Homebrew and Git

[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.