Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Johannes Sixt]

Am 29.03.2016 um 21:18 schrieb Johannes Sixt:

Am 28.03.2016 um 17:14 schrieb Johannes Schindelin:

The problem with your patch is that it does not account for backslashes in
paths resulting in quoting. I am afraid that your patch will most likely
*not* let the tests pass in Git for Windows SDK, while my patch does.


It does pass. The reason is that pwd -W generates forward slashes.


It just occurred to me that we might be observing a difference in 
behavior of pwd -W between the modern MSYS2 bash and the old MSYS1 bash 
that I am using. If that is the case the tests won't pass under MSYS2 
without the change made by 5ca6b7bb (config --show-origin: report paths 
with forward slashes).


-- Hannes

--
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?] retrying with "am -3" doesn't work anymore

[Jeff King]

On Wed, Mar 30, 2016 at 12:18:30PM +0800, Paul Tan wrote:

> On Wed, Mar 30, 2016 at 10:15 AM, Jeff King  wrote:
> > I noticed that I could not get a patch from Junio to apply earlier
> > today, and I think it is a regression in the builtin git-am
> > implementation.  I had trouble reproducing with a basic test case,
> > though.
> >
> > Basically, I picked up the three patches from this sub-thread:
> >
> >   http://thread.gmane.org/gmane.comp.version-control.git/288987/focus=290222
> >
> > and tried to apply them on top of v2.8.0.
> >
> > Doing it with "git am -3 patches" works. But doing it with:
> >
> >   git am patches
> >   git am -3
> >
> > doesn't. Bisecting shows that it did work before 783d7e8 (builtin-am:
> > remove redirection to git-am.sh, 2015-08-04).
> 
> Yeah, with "git am -3" when resuming, the -3 will only affect the
> current conflicting patch since 852a171 (am: let command-line options
> override saved options, 2015-08-04).

Ah, right. I had a nagging feeling that we had discussed this, and
indeed, I already participated in the discussion last July[1]. I even
apparently argued in favor of the new behavior[2]. Yikes. There goes my
mind.

Running "git am -3" for each patch which needs it does indeed
successfully apply the series.

Thanks for a quick response, and sorry for the noise.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/274574

[2] In 
http://thread.gmane.org/gmane.comp.version-control.git/274574/focus=274635,
though I think there I more meant that in:

   git am
   git am -3
   git am

the third one would not use "-3" again. So I was mostly confused
that in:

   git am
   git am -3

we would not use "-3" for the subsequent patches applied by that
second invocation. So maybe that is a bug. I dunno. I could see
arguments either way.
--
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: [ANNOUNCE] git-push-update, tool to push with "server-side" merge or rebase

[Max Kirillov]

On Tue, Mar 29, 2016 at 09:29:45PM -0400, Trevor Saunders wrote:
> hm how? the workflow you use locally has basically nothingto do with how
> pushes work.  I work on several projects daily where everyone pushes to
> trunk, but locally I use branches.  You just need to fetch rebase then
> either merge your branch into master before pushing or explicitly tell
> git push what refs to update how.

If user is confident in manipulating with branches then
probably this does not provide much value. Though it also
to some extent prevents from pushing to wrong branch by
mistake.

>> * when the trunk goes forward, user have to run merge or
>>   rebase (further "update"), interrupting other work which
>>   might be in progress.
> 
> I don't really understand this either, if you develope everything on
> master then it would seem obvious if you want to update what version of
> trunk you are using you either need to rebase or merge the remote master
> with yours.

Updating your current working branch is not free, if you
have a long compilation. Also new changes can break
something. 

In CVCS (think subversion) nobody really updates after each
commit to server from anybody. You 'keep uptodate' by
updating something like once a day. Otherwise don't have to
update unless somebody touches same file as you. I tried to
restore this opportunity.

>> * while doing fetch, update and push back a concurrent push can happen,
>>   making user to have to repeat it all over.
> 
> I think this is more or less the reason for the hg extension, but I
> think the script to deal with this is basically
> 
> while true
> do
>   git fetch origin
>   git rebase origin/master
>   git push origin HEAD:master && break
> done
> 
> obviously with a little more error checking thrown in if you care.

yes, basically push-update does not do much more than this.

>> This was discussed around some time ago, but I could not find anything
>> done about it. It might seem like nobody really interested much. But I
>> still can see discussions here and there. Also, some time ago extension
>> "pushrebase" for mercurial appeared, which indicates that there is
>> really a demand.
> 
> I think that was really for very heavily used repos where there was a
> ton of fetch rebase push repeating going on.

If does not have to be very heavy. Even small team (3-5
fulltime coders) can already feel a difference.

> I'm not really clear what this is helping for most of those use cases,
> but if you want to maintain it why not?

Let's see if anybody uses it. If somebody does then I can try.

-- 
Max
--
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: weird diff output?

[Jeff King]

On Tue, Mar 29, 2016 at 04:05:57PM -0700, Jacob Keller wrote:

> > This is what we want in both cases.
> > And I would argue it would appease many other kinds of text as well, because
> > an empty line is usually a strong indicator for any text that a
> > different thing comes along.
> > (Other programming languages, such as Java, C++ and any other C like
> > language behaves
> > that way; even when writing latex figures you'd rather want to break
> > at new lines?)
> >
> > Thanks,
> > Stefan
> 
> This seems like a good heuristic. Can we think of any examples where
> it would produce wildly confusing diffs? I don't think it necessarily
> needs to be default but just a possible option when formatting diffs,
> much like we already have today.

One thing I like to do when playing with new diff ideas is to pipe all
of "log -p" for a real project through it and see what differences it
produces.

Below is a perl script that implements Stefan's heuristic. I checked its
output on git.git with:

  git log --format='commit %H' -p >old
  perl /path/to/script new
  diff -F ^commit -u old new | less

which shows the differences, with the commit id in the hunk header
(which makes it easy to "git show $commit | perl /path/to/script" to
see the new diff with more context.

In addition to the cases discussed, it seems to improve C comments by
turning:

   /*
  + * new function
  + */
  +void foo(void);
  +
  +/*
* old function
...

into:

  +/*
  + * my function
  + */
  +void foo(void);
  +
   /*
* old function
...

See 47fe3f6e for an example.

It also seems to do OK with shell scripts. Commit e6bb5f78 is an example
where it improves a here-doc, as in the motivating example from this
thread. Similarly, the headers in 4df1e79 are much improved (though I'm
confused why the final one in that diff doesn't seem to have been
caught).

I also ran into an interesting case in 86d26f24, where we have:

  + test_expect_success '
  +   foo
  +
  +'
  +

and there are _two_ blank lines to choose from. It looks really terrible
if you use the first one, but the second one looks good (and the script
below chooses the second, as it's closest to the hunk boundary). There
may be cases where that's bad, though.

This is just a proof of concept. I guess we'd want to somehow integrate
the heuristic into git.

-- >8 --
#!/usr/bin/perl

use strict;
use warnings 'all';

use constant {
  STATE_NONE => 0,
  STATE_LEADING_CONTEXT => 1,
  STATE_IN_CHUNK => 2,
};
my $state = STATE_NONE;
my @hunk;
while(<>) {
  if ($state == STATE_NONE) {
print;
if (/^@/) {
  $state = STATE_LEADING_CONTEXT;
}
  } else {
if (/^ /) {
  flush_hunk() if $state != STATE_LEADING_CONTEXT;
  push @hunk, $_;
} elsif(/^[-+]/) {
  push @hunk, $_;
  $state = STATE_IN_CHUNK;
} else {
  flush_hunk();
  $state = STATE_NONE;
  print;
}
  }
}
flush_hunk();

sub flush_hunk {
  my $context_len = 0;
  while ($context_len < @hunk && $hunk[$context_len] =~ /^ /) {
$context_len++;
  }

  # Find the length of the ambiguous portion.
  # Assumes our hunks have context first, and ambiguous additions at the end,
  # which is how git generates them
  my $ambig_len = 0;
  while ($ambig_len < $context_len) {
my $i = $context_len - $ambig_len - 1;
my $j = @hunk - $ambig_len - 1;
if ($hunk[$j] =~ /^\+/ && substr($hunk[$i], 1) eq substr($hunk[$j], 1)) {
  $ambig_len++;
} else {
  last;
}
  }

  # Now look for an empty line in the ambiguous portion (we can just look in
  # the context side, as it is equivalent to the addition side at the end).
  # We count down, though, as we prefer to use the line closest to the
  # hunk as the cutoff.
  my $empty;
  for (my $i = $context_len - 1; $i >= $context_len - $ambig_len; $i--) {
if (length($hunk[$i]) == 2) {
  $empty = $i;
  last;
}
  }

  if (defined $empty) {
# move empty lines after the chunk to be part of it
for (my $i = $empty + 1; $i < $context_len; $i++) {
  $hunk[$i] =~ s/^ /+/;
  $hunk[@hunk - $context_len + $i] =~ s/^\+/ /;
}
  }

  print @hunk;
  @hunk = ();
}
--
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?] retrying with "am -3" doesn't work anymore

[Paul Tan]

Hi Jeff,

On Wed, Mar 30, 2016 at 10:15 AM, Jeff King  wrote:
> I noticed that I could not get a patch from Junio to apply earlier
> today, and I think it is a regression in the builtin git-am
> implementation.  I had trouble reproducing with a basic test case,
> though.
>
> Basically, I picked up the three patches from this sub-thread:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/288987/focus=290222
>
> and tried to apply them on top of v2.8.0.
>
> Doing it with "git am -3 patches" works. But doing it with:
>
>   git am patches
>   git am -3
>
> doesn't. Bisecting shows that it did work before 783d7e8 (builtin-am:
> remove redirection to git-am.sh, 2015-08-04).

Yeah, with "git am -3" when resuming, the -3 will only affect the
current conflicting patch since 852a171 (am: let command-line options
override saved options, 2015-08-04).

Regards,
Paul
--
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

git alias quoting help

[shawn wilson]

I've also tried to make this a plain bash script (w/o the function or
if statements and am failing at the same place). The issue seems to be
with the quoting in the filter-branch | ls-files bit. Also, the end
goal here is to be able to move a directory from one repo and keep the
history. While this works if I do it at the command line, it's just
too many steps (is tedious). Also, if there's a way to do the same
thing with multiple directories in one shot, (or make this work with
something like: cookbooks/{a,b,c} # as a parameter) that'd be perfect.

  reapdir = "!f() { \
if [ -d "$1" ] ; then \
  git filter-branch --prune-empty --subdirectory-filter "$1" -- --all && \
  git gc --aggressive && \
  git prune && \
  git filter-branch -f --prune-empty --index-filter '\
git ls-files -s \
  | sed \"s-\\t-&$1-\" \
  | GIT_INDEX_FILE=$GIT_INDEX_FILE.new git update-index
--index-info && \
mv $GIT_INDEX_FILE.new $GIT_INDEX_FILE'; \
else \
  echo "No directory $1"; \
fi; }; \
  f"
--
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

Message.

[Thomas Marr]

My Name is Thomas Marr and currently work as a Branch Operations Manager at 
Lichfield Finance Trust Bank, I wish to propose/discuss a profitable business 
opportunity to the tune of £5,732,000.00GBP with you, do indicate your 
willingness to partner with me for more information. Regards, Thomas Marr.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Zhang Lei]

Thanks for the review.
Sorry for the patch churn, I wasn't quite familiar with working with
mailing list.

2016-03-30 3:20 GMT+08:00 Eric Sunshine :
> On Tue, Mar 29, 2016 at 6:11 AM, Ray Zhang  wrote:
>> By adding this option which defaults to true, we can use the
>> corresponding --no-checkout to make some customizations before
>> the checkout, like sparse checkout, etc.
>>
>> Reviewed-by: Eric Sunshine 
>> Signed-off-by: Ray Zhang 
>> ---
>> Changes since last version of this patch[v4]:
>> t/t2025-worktree-add.sh: use test -e to test file existence.
>> builtin/worktree.c: refactor the code a little bit.
>
> Thanks, this version is still:
>
> Reviewed-by: Eric Sunshine 
>
> A couple comments below...
>
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -284,18 +285,22 @@ static int add_worktree(const char *path, const char 
>> *refname,
>> if (ret)
>> goto done;
>>
>> -   cp.argv = NULL;
>> -   argv_array_clear();
>> -   argv_array_pushl(, "reset", "--hard", NULL);
>> -   cp.env = child_env.argv;
>> -   ret = run_command();
>> -   if (!ret) {
>> -   is_junk = 0;
>> -   free(junk_work_tree);
>> -   free(junk_git_dir);
>> -   junk_work_tree = NULL;
>> -   junk_git_dir = NULL;
>> +   if (opts->checkout) {
>> +   cp.argv = NULL;
>> +   argv_array_clear();
>> +   argv_array_pushl(, "reset", "--hard", NULL);
>> +   cp.env = child_env.argv;
>> +   ret = run_command();
>> +   if (ret)
>> +   goto done;
>> }
>> +
>> +   is_junk = 0;
>> +   free(junk_work_tree);
>> +   free(junk_git_dir);
>> +   junk_work_tree = NULL;
>> +   junk_git_dir = NULL;
>
> Doing the goto-dance and outdenting the "freeing" code as suggested as
> a possible improvement by [1] probably should have been done as a
> separate preparatory patch since the result in this patch is fairly
> noisy and more difficult to review. However, it's probably not worth
> the patch churn to do so now.
>
>>  done:
>> strbuf_reset();
>> strbuf_addf(, "%s/locked", sb_repo.buf);
>> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
>> +test_expect_success '"add" worktree with --no-checkout' '
>> +   git worktree add --no-checkout -b swamp swamp &&
>> +   ! test -e swamp/init.t &&
>
> I realize that this was suggested by [2], however, a more modern way
> to state this would be:
>
> test_path_is_missing swamp/init.t &&
>
> but, as also mentioned in [2], it's probably not worth the patch churn
> to change it now.
>
>> +   git -C swamp reset --hard &&
>> +   test_cmp init.t swamp/init.t
>> +'
>> +
>> +test_expect_success '"add" worktree with --checkout' '
>> +   git worktree add --checkout -b swmap2 swamp2 &&
>> +   test_cmp init.t swamp2/init.t
>> +'
>> +
>>  test_done
>
> [1]: 
> http://git.661346.n2.nabble.com/PATCH-add-option-n-no-checkout-to-git-worktree-add-tp7651385p7651884.html
>
> [2]: http://article.gmane.org/gmane.comp.version-control.git/290050
--
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

Cheap Solid Wood Beds For Sale Salford UK

[vivocuy]

Cheap Solid Wood Beds For Sale Salford UK. Find Solid Wood Bed suppliers at
wewewe [dot] s o l i d w o o d b e d s [dot] c o [dot] u k



--
View this message in context: 
http://git.661346.n2.nabble.com/Cheap-Solid-Wood-Beds-For-Sale-Salford-UK-tp7651989.html
Sent from the git mailing list archive at Nabble.com.
--
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

[BUG?] retrying with "am -3" doesn't work anymore

[Jeff King]

I noticed that I could not get a patch from Junio to apply earlier
today, and I think it is a regression in the builtin git-am
implementation.  I had trouble reproducing with a basic test case,
though.

Basically, I picked up the three patches from this sub-thread:

  http://thread.gmane.org/gmane.comp.version-control.git/288987/focus=290222

and tried to apply them on top of v2.8.0.

Doing it with "git am -3 patches" works. But doing it with:

  git am patches
  git am -3

doesn't. Bisecting shows that it did work before 783d7e8 (builtin-am:
remove redirection to git-am.sh, 2015-08-04).

I tried to tweak t4150 to show this by replacing a "git am -3 foo" with
"test_must_fail git am foo && git am -3", but it failed even on older
git, which made me think I fumbled the test somehow.

-Peff
--
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 v4 2/3] pretty: enable --expand-tabs by default for selected pretty formats

[Jeff King]

On Tue, Mar 29, 2016 at 04:15:08PM -0700, Junio C Hamano wrote:

> diff --git a/Documentation/pretty-options.txt 
> b/Documentation/pretty-options.txt
> index 4fb5c76..23967b6 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -43,10 +43,16 @@ people using 80-column terminals.
>   commit may be copied to the output.
>  
>  --expand-tabs::
> +--no-expand-tabs::
>   Perform a tab expansion (replace each tab with enough number
>   of spaces to fill to the next display column that is
>   multiple of 8) in the log message before using the message
>   to show in the output.
> ++
> +By default, tabs are expanded in pretty formats that indent the log
> +message by 4 spaces (i.e.  'medium', which is the default, 'full',
> +and "fuller').  `--no-expand-tabs` option can be used to disable
> +this.

Mismatched quote types on "fuller".

> @@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info 
> *rev)
>  
>   rev->commit_format = commit_format->format;
>   rev->use_terminator = commit_format->is_tformat;
> + rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
>   if (commit_format->format == CMIT_FMT_USERFORMAT) {
>   save_user_format(rev, commit_format->user_format,
>commit_format->is_tformat);

This feels like the wrong time to set the value in rev_info, as it means
that:

  git log --no-expand-tabs --pretty=full

and

  git log --pretty=full --no-expand-tabs

behave differently. The other values set in get_commit_format, like
"use_terminator", are inherently part of the format, but I don't think
this is. We just want to set the default if the user did not express
another preference.

Likewise, if we were to eventually add config like "[log]expandtab = 4",
it should not be overridden by "--pretty=full" (but we probably _would_
want to have it kick in only for certain formats).

So I think we really want to set an alternate variable here, and then do
something like:

  if (rev->expand_tabs_in_log < 0)
rev->expand_tabs_in_log = rev->commit_format_expand_tab_default;

-Peff
--
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: [ANNOUNCE] git-push-update, tool to push with "server-side" merge or rebase

[Trevor Saunders]

On Mon, Mar 28, 2016 at 11:08:41AM +0300, Max Kirillov wrote:
> Hello.
> 
> I would like to announce git-push-update, a tool which emulates
> server-side merge or rebase.
> 
> The link: https://github.com/max630/git-push-update
> 
> It is a bash script which fetches latest remote branch, creates
> temporary clone, performs there merge or rebase, pushes results to
> server. If during the merge or rebase remote branch was updated, it
> fails cleanly, so you are not left with merge which did not go anywhere.
> Or it can even retry the whole task from the beginning, until it
> eventually succeeds.
> 
> I tried to make it easy to use by unexperienced users by making as few
> options as possible and checking for some dangerous mistakes. It would
> be nice if somebody tried to really use it, so there would be some data
> does this direction worth exploring. Any other feedback is also welcome.
> 
> A longer explanation:
> 
> While topic branches/pullrequests/whatever-it-named workflow is
> obviously superior to push-to-trunk approach which is used with
> centralized VCSes, there can be cases to use the latter one. But doing
> it with Git is not easy:

hm how? the workflow you use locally has basically nothingto do with how
pushes work.  I work on several projects daily where everyone pushes to
trunk, but locally I use branches.  You just need to fetch rebase then
either merge your branch into master before pushing or explicitly tell
git push what refs to update how.

> * when the trunk goes forward, user have to run merge or
>   rebase (further "update"), interrupting other work which
>   might be in progress.

I don't really understand this either, if you develope everything on
master then it would seem obvious if you want to update what version of
trunk you are using you either need to rebase or merge the remote master
with yours.

> * while doing fetch, update and push back a concurrent push can happen,
>   making user to have to repeat it all over.

I think this is more or less the reason for the hg extension, but I
think the script to deal with this is basically

while true
do
git fetch origin
git rebase origin/master
git push origin HEAD:master && break
done

obviously with a little more error checking thrown in if you care.

> * some scenarios allow user to make a mistake combining branches which
>   mean to be unrelated, for example merging or rebasing active
>   development branch into maintenance branch for older version.
> * for a merge case there is a problem of "evil pull"
>   (http://thread.gmane.org/gmane.comp.version-control.git/247237)
>   In short: the merges which are to go to remote branch should be "from
>   local to remote", and git-pull merges "from remote to local".
> 
> This was discussed around some time ago, but I could not find anything
> done about it. It might seem like nobody really interested much. But I
> still can see discussions here and there. Also, some time ago extension
> "pushrebase" for mercurial appeared, which indicates that there is
> really a demand.

I think that was really for very heavily used repos where there was a
ton of fetch rebase push repeating going on.

> Looks like current git remote protocol does not really allow server to
> tweak pushed commits: if it accepts reference, client will remember
> exactly the commit it was pushing, no modifications is possible. Also,
> if it is implemented as server-side feature, it might take years to
> appear at github or other big public hostings, if ever. Until that most
> users would not be able to try it.

I'm not really clear what this is helping for most of those use cases,
but if you want to maintain it why not?

Trev

> This leaves only one option: perform merge or rebase locally, pretending
> that it was done at server. It does not even have to be implemented in
> git itself, instead a wrapper script can do everything.
> 
> So here is the script.
> 
> -- 
> Max
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] abbrev_sha1_in_line: don't leak memory

[Jeff King]

On Tue, Mar 29, 2016 at 09:30:38PM -0400, Eric Sunshine wrote:

> On Tue, Mar 29, 2016 at 9:11 PM, Jeff King  wrote:
> > On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:
> >> `split` is of type `struct strbuf **` and just before the new free,
> >> we release the inner strbufs. Make sure to also release the memory
> >> containing the pointers to the individual strbufs.
> >>
> >> Signed-off-by: Stefan Beller 
> >> ---
> >> diff --git a/wt-status.c b/wt-status.c
> >> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
> >>   }
> >>   for (i = 0; split[i]; i++)
> >>   strbuf_release(split[i]);
> >> -
> >> + free(split);
> >>  }
> >
> > I think this can just combine with the for-loop above to become
> > strbuf_list_free().
> 
> The implementation of strbuf_list_free() is this:
> 
> struct strbuf **s = sbs;
> while (*s) {
> strbuf_release(*s);
> free(*s++);
> }
> free(sbs);
> 
> which means that wt-status.c is leaking not only 'split', but also
> each element of split[], right?

Yeah, I didn't notice that, but I think you're right.

-Peff
--
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 4/6] abbrev_sha1_in_line: don't leak memory

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:11 PM, Jeff King  wrote:
> On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:
>> `split` is of type `struct strbuf **` and just before the new free,
>> we release the inner strbufs. Make sure to also release the memory
>> containing the pointers to the individual strbufs.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>> diff --git a/wt-status.c b/wt-status.c
>> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
>>   }
>>   for (i = 0; split[i]; i++)
>>   strbuf_release(split[i]);
>> -
>> + free(split);
>>  }
>
> I think this can just combine with the for-loop above to become
> strbuf_list_free().

The implementation of strbuf_list_free() is this:

struct strbuf **s = sbs;
while (*s) {
strbuf_release(*s);
free(*s++);
}
free(sbs);

which means that wt-status.c is leaking not only 'split', but also
each element of split[], right?
--
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

[PATCHv4 5/6] submodule update: test recursive path reporting from subdirectory

[Stefan Beller]

This patch is just a test and fixes no bug as there is currently no bug
in the path handling of `submodule update`.

In `submodule update` we make a call to `submodule--helper list --prefix
"$wt_prefix"` which looks a bit brittle and likely to introduce a bug
for the path handling. It is not a bug as the prefix is ignored inside
the submodule helper for now. If this test breaks eventually, we want
to make sure the `wt_prefix` is passed correctly into recursive submodules.
Hint: In recursive submodules we expect `wt_prefix` to be empty.

Signed-off-by: Stefan Beller 
---
 t/t7406-submodule-update.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 01dd324..e5af4b4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -379,6 +379,26 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_cmp actual expect
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 
'../super/submodule'
+Failed to recurse into submodule path '../super'
+EOF
+
+test_expect_success 'recursive submodule update - command in .git/config 
catches failure -- subdirectory' '
+   (cd recursivesuper &&
+git submodule update --remote super &&
+git add super &&
+git commit -m "update to latest to have more than one commit in 
submodules"
+   ) &&
+   git -C recursivesuper/super config submodule.submodule.update "!false" 
&&
+   git -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&
+   (cd recursivesuper &&
+mkdir -p tmp && cd tmp &&
+test_must_fail git submodule update --recursive ../super 2>../../actual
+   ) &&
+   test_cmp actual expect
+'
+
 test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
 H=$(git ls-files -s submodule | cut -d" " -f2) &&
-- 
2.8.0.2.gb331331

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

[PATCHv4 6/6] t7407: make expectation as clear as possible

[Stefan Beller]

Not everyone (including me) grasps the sed expression in a split second as
they would grasp the 4 lines printed as is.

Signed-off-by: Stefan Beller 
---
 t/t7407-submodule-foreach.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 4b35e12..6ba5daf 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
 '
 
-sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" 
< expect > expect2
-mv -f expect2 expect
+cat > expect 

[PATCHv4 3/6] submodule status: correct path handling in recursive submodules

[Stefan Beller]

The new test which is a replica of the previous test except
that it executes from a sub directory. Prior to this patch
the test failed by having too many '../' prefixed:

  --- expect2016-03-29 19:02:33.087336115 +
  +++ actual2016-03-29 19:02:33.359343311 +
  @@ -1,7 +1,7 @@
b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
  -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
  - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 
(heads/master)
  - 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../nested1/nested2/nested3/submodule (heads/master)
  ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
  + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 
(heads/master)
  + 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../../../../nested1/nested2/nested3/submodule (heads/master)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)

The path code in question:
  displaypath=$(relative_path "$prefix$sm_path")
  prefix=$displaypath
  if recursive:
eval cmd_status

That way we change `prefix` each iteration to contain another
'../', because of the the relative_path computation is done
on an already computed relative path.

We must call relative_path exactly once with `wt_prefix` non empty.
Further calls in recursive instances to to calculate the displaypath
already incorporate the correct prefix from before. Fix the issue by
clearing `wt_prefix` in recursive calls.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  1 +
 t/t7407-submodule-foreach.sh | 21 +
 2 files changed, 22 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index 9fffa5c..1024774 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1159,6 +1159,7 @@ cmd_status()
(
prefix="$displaypath/"
clear_local_git_env
+   wt_prefix=
cd "$sm_path" &&
eval cmd_status
) ||
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..4b35e12 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" 
preserves the --cached
test_cmp expect actual
 '
 
+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
+
+cat > expect < ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
-- 
2.8.0.2.gb331331

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

[PATCHv4 2/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

When calling `git submodule init` from a recursive instance of
`git submodule update --recursive`, the reported path is wrong as it
skips the nested submodules.

The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
Submodule 'submodule' (${pwd}/submodule) registered for path 
'../super/submodule'
the `super` directory is omitted and you get
Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.

Signed-off-by: Stefan Beller 
---

Notes:
* Use `git -C ` instead of `cd  && git`.
* Reword commit message

 git-submodule.sh|  2 +-
 t/t7406-submodule-update.sh | 33 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index b3f248c..9fffa5c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -473,7 +473,7 @@ cmd_init()
die_if_unmatched "$mode"
name=$(git submodule--helper name "$sm_path") || exit
 
-   displaypath=$(relative_path "$sm_path")
+   displaypath=$(relative_path "$prefix$sm_path")
 
# Copy url setting when it is not set yet
if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..d4745f4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
 git submodule add ../none none &&
 test_tick &&
 git commit -m "none"
+   ) &&
+   git clone . recursivesuper &&
+   ( cd recursivesuper
+git submodule add ../super super
)
 '
 
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
)
 '
 
+supersha1=$(git -C super rev-parse HEAD)
+mergingsha1=$(git -C super/merging rev-parse HEAD)
+nonesha1=$(git -C super/none rev-parse HEAD)
+rebasingsha1=$(git -C super/rebasing rev-parse HEAD)
+submodulesha1=$(git -C super/submodule rev-parse HEAD)
+pwd=$(pwd)
+
+cat ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 apos="'";
 test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
-- 
2.8.0.2.gb331331

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

[PATCHv4 4/6] submodule update: align reporting path for custom command execution

[Stefan Beller]

In the predefined actions (merge, rebase, none, checkout), we use
the display path, which is relative to the current working directory.
Also use the display path when running a custom command.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  4 ++--
 t/t7406-submodule-update.sh | 29 ++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 1024774..753a90d 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -802,8 +802,8 @@ Maybe you want to use 'update --init'?")"
;;
!*)
command="${update_module#!}"
-   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
-   say_msg="$(eval_gettext "Submodule path 
'\$prefix\$sm_path': '\$command \$sha1'")"
+   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$displaypath'")"
+   say_msg="$(eval_gettext "Submodule path 
'\$displaypath': '\$command \$sha1'")"
must_die_on_failure=yes
;;
*)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index d4745f4..01dd324 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in 
.git/config' '
)
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 'submodule'
+EOF
+
 test_expect_success 'submodule update - command in .git/config catches 
failure' '
(cd super &&
 git config submodule.submodule.update "!false"
) &&
(cd super/submodule &&
- git reset --hard HEAD^
+ git reset --hard $submodulesha1^
) &&
(cd super &&
-test_must_fail git submodule update submodule
-   )
+test_must_fail git submodule update submodule 2>../actual
+   ) &&
+   test_cmp actual expect
+'
+
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../submodule'
+EOF
+
+test_expect_success 'submodule update - command in .git/config catches failure 
-- subdirectory' '
+   (cd super &&
+git config submodule.submodule.update "!false"
+   ) &&
+   (cd super/submodule &&
+ git reset --hard $submodulesha1^
+   ) &&
+   (cd super &&
+mkdir tmp && cd tmp &&
+test_must_fail git submodule update ../submodule 2>../../actual
+   ) &&
+   test_cmp actual expect
 '
 
 test_expect_success 'submodule init does not copy command into .git/config' '
-- 
2.8.0.2.gb331331

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

[PATCHv4 1/6] submodule foreach: correct path display in recursive submodules

[Stefan Beller]

The `prefix` was put in front of the display path unconditionally.
This is wrong as any relative path computation would need to be at
the front, so include the prefix into the display path.

The new test replicates the previous test with the difference of executing
from a sub directory. By executing from a sub directory all we would
expect all displayed paths to be prefixed by '../'.

Prior to this patch the test would report
Entering 'nested1/nested2/../nested3'
instead of the expected
Entering '../nested1/nested2/nested3'

Signed-off-by: Stefan Beller 
---

Notes:
* reworded commit message
* when writing the commit message I discovered a new way to fix the bug
  (fix the computation of the displaypath instead of its parameters
   wt_prefix and prefix.)

  The result is the same, but I am not yet sure if I like it more.

 git-submodule.sh |  6 +++---
 t/t7407-submodule-foreach.sh | 20 
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..b3f248c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -413,8 +413,8 @@ cmd_foreach()
die_if_unmatched "$mode"
if test -e "$sm_path"/.git
then
-   displaypath=$(relative_path "$sm_path")
-   say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
+   displaypath=$(relative_path "$prefix$sm_path")
+   say "$(eval_gettext "Entering '\$displaypath'")"
name=$(git submodule--helper name "$sm_path")
(
prefix="$prefix$sm_path/"
@@ -434,7 +434,7 @@ cmd_foreach()
cmd_foreach "--recursive" "$@"
fi
) <&3 3<&- ||
-   die "$(eval_gettext "Stopping at 
'\$prefix\$displaypath'; script returned non-zero status.")"
+   die "$(eval_gettext "Stopping at '\$displaypath'; 
script returned non-zero status.")"
fi
done
 }
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach 
--recursive"' '
 '
 
 cat > expect <../../actual
+   ) &&
+   test_i18ncmp expect actual
+'
+
+cat > expect 

[PATCHv4 0/6] Fix path bugs in submodule commands executed from sub dir

[Stefan Beller]

v4:
* addressed Junios comments in patch 1&2, which is:
1)
* reworded commit message
* when writing the commit message I discovered a new way to fix the bug
  (fix the computation of the displaypath instead of its parameters
   wt_prefix and prefix.)

  The result is the same, but I am not yet sure if I like it more.

2)
* Use `git -C ` instead of `cd  && git`.
* Reword commit message

v3:
Resending without `test_pause` in the last test.

v2:

The first 3 commits are
* Test and bugfix in one commit each
* better explained than before

The expansion of an expected test result moved to the back of the series.

There are two new commits
* one being another bugfix of the display path for `submodule update`
* another test for `submodule update` as I suspect it may break further on
  refactoring and we currently have no tests for it.

Thanks,
Stefan

Stefan Beller (6):
  submodule foreach: correct path display in recursive submodules
  submodule update --init: correct path handling in recursive submodules
  submodule status: correct path handling in recursive submodules
  submodule update: align reporting path for custom command execution
  submodule update: test recursive path reporting from subdirectory
  t7407: make expectation as clear as possible

 git-submodule.sh | 13 +++
 t/t7406-submodule-update.sh  | 82 ++--
 t/t7407-submodule-foreach.sh | 49 --
 3 files changed, 133 insertions(+), 11 deletions(-)

-- 
2.8.0.2.gb331331

--
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 6/6] credential-cache, send_request: close fd when done

[Jeff King]

On Tue, Mar 29, 2016 at 05:38:53PM -0700, Stefan Beller wrote:

> No need to keep it open any further.
> 
> Signed-off-by: Stefan Beller 
> ---
>  credential-cache.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/credential-cache.c b/credential-cache.c
> index f4afdc6..86e21de 100644
> --- a/credential-cache.c
> +++ b/credential-cache.c
> @@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct 
> strbuf *out)
>   write_or_die(1, in, r);
>   got_data = 1;
>   }
> + close(fd);
>   return got_data;
>  }

Looks good. I think nobody ever noticed because credential-cache
basically sends off the request and then exits.

-Peff
--
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/6] bundle: don't leak an fd in case of early return

[Jeff King]

On Tue, Mar 29, 2016 at 05:38:52PM -0700, Stefan Beller wrote:

> In successful operation `write_pack_data` will close the `bundle_fd`,
> but when we exit early, we need to take care of the file descriptor
> ourselves.
> 
> Signed-off-by: Stefan Beller 
> ---
>  bundle.c | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 506ac49..04d62af 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   init_revisions(, NULL);
>  
>   /* write prerequisites */
> - if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
> + if (compute_and_write_prerequisites(bundle_fd, , argc, argv)) {
> + if (!bundle_to_stdout)
> + close(bundle_fd);
>   return -1;
> + }

Makes sense. Should we also be rolling back the lock file? It happens
automatically at program exit, of course, but we are in library code
here that should not rely on that.

> @@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const 
> char *path,
>   ref_count = write_bundle_refs(bundle_fd, );
>   if (!ref_count)
>   die(_("Refusing to create empty bundle."));
> - else if (ref_count < 0)
> + else if (ref_count < 0) {
> + if (!bundle_to_stdout)
> + close(bundle_fd);
>   return -1;
> + }

Ditto here, and in the error return from write_pack_data(). There are
enough spots that it may be worth setting up a "goto err" path.

-Peff
--
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/6] notes: don't leak memory in git_config_get_notes_strategy

[Jeff King]

On Tue, Mar 29, 2016 at 05:38:50PM -0700, Stefan Beller wrote:

> `value` is just a temporary scratchpad, so we need to make sure it doesn't
> leak. It is xstrdup'd in `git_config_get_string_const` and
> `parse_notes_merge_strategy` just compares the string against predefined
> values, so no need to keep it around longer.
> 
> Signed-off-by: Stefan Beller 
> ---
>  builtin/notes.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/notes.c b/builtin/notes.c
> index ed6f222..d6bab17 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,
>   if (parse_notes_merge_strategy(value, strategy))
>   git_die_config(key, "unknown notes merge strategy %s", value);
>  
> + free((void*)value);
>   return 0;
>  }

I don't think this is wrong, but would it perhaps be simpler to call
git_config_get_value() in the first place, which does not make a copy of
the string?

-Peff
--
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/6] notes: don't leak memory in git_config_get_notes_strategy

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller  wrote:
> `value` is just a temporary scratchpad, so we need to make sure it doesn't
> leak. It is xstrdup'd in `git_config_get_string_const` and
> `parse_notes_merge_strategy` just compares the string against predefined
> values, so no need to keep it around longer.
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/builtin/notes.c b/builtin/notes.c
> @@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,
> if (parse_notes_merge_strategy(value, strategy))
> git_die_config(key, "unknown notes merge strategy %s", value);
>
> +   free((void*)value);

I wonder if the intent would be clearer if you gave 'value' type 'char
*' rather than 'const char *', and called git_config_get_string()
rather than git_config_get_string_const().

> return 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 4/6] abbrev_sha1_in_line: don't leak memory

[Jeff King]

On Tue, Mar 29, 2016 at 05:38:51PM -0700, Stefan Beller wrote:

> `split` is of type `struct strbuf **` and just before the new free,
> we release the inner strbufs. Make sure to also release the memory
> containing the pointers to the individual strbufs.
> 
> Signed-off-by: Stefan Beller 
> ---
>  wt-status.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/wt-status.c b/wt-status.c
> index ef74864..a78cfc0 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
>   }
>   for (i = 0; split[i]; i++)
>   strbuf_release(split[i]);
> -
> + free(split);
>  }

I think this can just combine with the for-loop above to become
strbuf_list_free().

-Peff
--
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/6] imap-send.c, cram: allocate enough memory for null terminated string

[Jeff King]

On Tue, Mar 29, 2016 at 05:38:49PM -0700, Stefan Beller wrote:

> `strlen` returns the length of a string without the terminating null byte.
> To make sure enough memory is allocated we need to pass `strlen(..) + 1`
> to the allocation function.
> 
> Signed-off-by: Stefan Beller 
> ---
>  imap-send.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/imap-send.c b/imap-send.c
> index 2c52027..f7e9909 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char 
> *user, const char *pass)
>* enough upper bound for challenge (decoded result).
>*/
>   encoded_len = strlen(challenge_64);
> - challenge = xmalloc(encoded_len);
> + challenge = xmalloc(encoded_len + 1);
>   decoded_len = EVP_DecodeBlock((unsigned char *)challenge,
> (unsigned char *)challenge_64, 
> encoded_len);
>   if (decoded_len < 0)

Others pointed out that patches 1 and 2 here probably don't need the
extra byte. But as a side note, for any cases that do, please use
xmallocz() instead of manually adding 1 to the length. Even if you don't
need the final byte to be NUL, it checks for integer overflow.

-Peff
--
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-apply does not work in a sub-directory of a Git repository

[Duy Nguyen]

On Thu, Mar 24, 2016 at 11:50 PM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>>> On Wed, Mar 23, 2016 at 11:55 PM, Junio C Hamano  wrote:
 The include/exclude mechanism does use wildmatch() but does not use
 the pathspec mechanism (it predates the pathspec machinery that was
 made reusable in places like this).  We should be able to

 $ cd d/e/e/p/d/i/r
 $ git apply --include=:/ ../../../../../../../patch

 to lift this limitation.  IOW, we can think of the use_patch() to
 include only the paths in the subdirectory we are in by default, but
 we can make it allow --include/--exclude command line option to
 override that default.
>>
>> I went with a new option instead of changing --include.
>
> It might be a "workable" band-aid, but would be an unsatisfying UI
> if it were the endgame state.  You do not say "git grep --whole" (by
> the way, "whole" is a bad option name, as you cannot tell "100% of
> *what*" you are referring to--what you are widening is the limit
> based on the location in the directory structure, so the option name
> should have some hint about it, e.g. "full-tree" or something) and
> this command will become an odd-man-out.
>
> I haven't thought things through, but thinking out aloud a few
> points...
>
>   An existing user/script may be working in a subdirectory of a huge
>   working tree and relies on the current behaviour that outside world
>   is excluded by default, and may be passing --exclude to further
>   limit the extent of damage by applying the patch to a subset of
>   paths in the current directory that itself is also huge.  And that
>   use case would not be harmed by such a change.
>
>   On the other hand, an existing user/script would not be passing an
>   "--include" that names outside the current subdirectory to force
>   them to be included, because it is known for the past 10 years not
>   to have any effect at all.

Real-world .gitignore patterns have taught me that even if it does not
have any effect, it might still be present in some scripts, waiting
for a chance to bite me.

> So a better alternative may be to conditionally disable the "Paths
> outside are not touched regardless of --include" logic, i.e. we
> exclude paths outside by default just as before, but if there is at
> least one explicit "--include" given, we skip this "return 0".
>
> That way, we do not have to commit to turning --include/--exclude to
> pathspec (which I agree is a huge change in behaviour that may not
> be a good idea) and we do not have to add "--full-tree" that is only
> understood by "apply" but not other commands that operate on the
> current directory by default.

But your suggestion is good and I can't think of any better. We could
introduce pathspec as ftiler after "--", but it does not look elegant,
and it overlaps with --include/--exclude.

Perhaps we can start to warn people if --include is specified but has
no effect for a cycle or two, then we can do as you suggested?
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/6] imap-send.c, cram: allocate enough memory for null terminated string

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller  wrote:
> `strlen` returns the length of a string without the terminating null byte.
> To make sure enough memory is allocated we need to pass `strlen(..) + 1`
> to the allocation function.
>
> Signed-off-by: Stefan Beller 
> ---
>  imap-send.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/imap-send.c b/imap-send.c
> index 2c52027..f7e9909 100644
> --- a/imap-send.c
> +++ b/imap-send.c
> @@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char 
> *user, const char *pass)
>  * enough upper bound for challenge (decoded result).
>  */
> encoded_len = strlen(challenge_64);
> -   challenge = xmalloc(encoded_len);
> +   challenge = xmalloc(encoded_len + 1);

'challenge' is never used as a NUL-terminated string, and
EVP_DecodeBlock() is not advertised as adding a terminating NUL, so
why is this an improvement?

> decoded_len = EVP_DecodeBlock((unsigned char *)challenge,
>   (unsigned char *)challenge_64, 
> encoded_len);
> if (decoded_len < 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 2/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

On Tue, Mar 29, 2016 at 4:54 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> The new test demonstrates a failure in the code prior to this patch.
>> Instead of getting the expected
>> Submodule 'submodule' (${pwd}/submodule) registered for path 
>> '../super/submodule'
>> the `super` directory is omitted and you get
>> Submodule 'submodule' (${pwd}/submodule) registered for path 
>> '../submodule'
>> instead.
>
> Same "is this about test?" comment applies here.
>
>> That happens because the prefix is ignored in `git submodule add`, probably
>> because that function itself cannot recurse;
>
> "probably"???

I am speculating there. I have no idea why it originally was written that way
but I would assume this is the most likeliest explanation.
I'll reword the whole commit message.

>
>>
>> +supersha1=$(cd super && git rev-parse HEAD)
>
> Perhaps "git -C super rev-parse HEAD"?

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 1/6] path.c: allocate enough memory for string

[Junio C Hamano]

Stefan Beller  writes:

> `strlen` returns the length of a string without the terminating null byte.
> To make sure enough memory is allocated we need to pass `strlen(..) + 1`
> to the allocation function.
>
> Signed-off-by: Stefan Beller 
> ---
>  path.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/path.c b/path.c
> index 969b494..0ae8af5 100644
> --- a/path.c
> +++ b/path.c
> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void 
> *value)
>   struct trie *new_node = xcalloc(1, sizeof(*new_node));
>   new_node->len = strlen(key);
>   if (new_node->len) {
> - new_node->contents = xmalloc(new_node->len);
> + new_node->contents = xmalloc(new_node->len + 1);
>   memcpy(new_node->contents, key, new_node->len);
>   }

This structure looks like a counted string  that does not
want to have a terminating NUL after the contents, judging from the
way memcpy() copies only len and not len+1.

Did I write this (wondering why this was addressed to me)?

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

Re: [PATCH 1/6] path.c: allocate enough memory for string

[Eric Sunshine]

On Tue, Mar 29, 2016 at 8:38 PM, Stefan Beller  wrote:
> `strlen` returns the length of a string without the terminating null byte.
> To make sure enough memory is allocated we need to pass `strlen(..) + 1`
> to the allocation function.
>
> Signed-off-by: Stefan Beller 
> ---
> diff --git a/path.c b/path.c
> @@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void 
> *value)
> struct trie *new_node = xcalloc(1, sizeof(*new_node));
> new_node->len = strlen(key);
> if (new_node->len) {
> -   new_node->contents = xmalloc(new_node->len);
> +   new_node->contents = xmalloc(new_node->len + 1);
> memcpy(new_node->contents, key, new_node->len);

Huh? This is a trie. It never accesses 'contents' as a NUL-terminated
string. Plus, no NUL is ever even copied, thus this is just
overallocating. How is this an improvement?

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

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

[Duy Nguyen]

On Wed, Mar 30, 2016 at 3:34 AM, Jeff King  wrote:
> On Tue, Mar 29, 2016 at 06:42:44AM -0500, Elliott Cable wrote:
>
>> So, I find this behaviour a little strange; I can't determine if it's
>> a subtle bug, or intentionally undefined/‘fuzzy’ behaviour:
>>
>> $ cd a-repo/.git/
>> $ pwd
>> /path/to/a-repo/.git
>> $ git rev-parse --is-inside-work-tree
>> false
>> $ export GIT_WORK_TREE=/path/to/a-repo
>> $ git rev-parse --is-inside-work-tree
>> true
>>
>> i.e. when within the repository (the `.git` directory), and when that
>> directory is a sub-directory of the working-tree, `rev-parse
>> --is-inside-work-tree` reports *false* (reasonable enough, I suppose);
>> but then if `$GIT_WORK_TREE` is set to precisely the directory that
>> git was *already* assuming was the working-directory, then the same
>> command, in the same location, reports *true*.
>>
>> This should probably be made consistent: either `rev-parse
>> --is-inside-work-tree` should report “true”, even inside the `.git`
>> dir, as long as that directory is a sub-directory of the working-tree
>> … or repository-directories / `$GIT_DIR` / `.git` directories should
>> be excluded from truthy responses to `rev-parse
>> --is-inside-work-tree`.

No. Once you set GIT_WORK_TREE you tell git that worktree exists. That
overrides the "bare repo" attribute (i.e. no worktree) that's
automatically set when we try to find .git directory.

> Yeah, I think this is a bug. Presumably what is happening is that we are
> too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> we ask "are we in a work tree", the answer has become yes. But the
> caller really wants to know "am _I_ inside the work tree".

On relative GIT_WORK_TREE some mail down this thread, there's this
note in t1510 that you might find interesting

5. GIT_WORK_TREE/core.worktree was originally meant to work only if
   GIT_DIR is set, but earlier git didn't enforce it, and some scripts
   depend on the implementation that happened to first discover .git by
   going up from the users $cwd and then using the specified working tree
   that may or may not have any relation to where .git was found in.  This
   historical behaviour must be kept.

Basically if you set GIT_WORK_TREE you better set GIT_DIR too to keep
things sane.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 6/6] credential-cache, send_request: close fd when done

[Stefan Beller]

No need to keep it open any further.

Signed-off-by: Stefan Beller 
---
 credential-cache.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/credential-cache.c b/credential-cache.c
index f4afdc6..86e21de 100644
--- a/credential-cache.c
+++ b/credential-cache.c
@@ -32,6 +32,7 @@ static int send_request(const char *socket, const struct 
strbuf *out)
write_or_die(1, in, r);
got_data = 1;
}
+   close(fd);
return got_data;
 }
 
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] abbrev_sha1_in_line: don't leak memory

[Stefan Beller]

`split` is of type `struct strbuf **` and just before the new free,
we release the inner strbufs. Make sure to also release the memory
containing the pointers to the individual strbufs.

Signed-off-by: Stefan Beller 
---
 wt-status.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index ef74864..a78cfc0 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1065,7 +1065,7 @@ static void abbrev_sha1_in_line(struct strbuf *line)
}
for (i = 0; split[i]; i++)
strbuf_release(split[i]);
-
+   free(split);
 }
 
 static void read_rebase_todolist(const char *fname, struct string_list *lines)
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] bundle: don't leak an fd in case of early return

[Stefan Beller]

In successful operation `write_pack_data` will close the `bundle_fd`,
but when we exit early, we need to take care of the file descriptor
ourselves.

Signed-off-by: Stefan Beller 
---
 bundle.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/bundle.c b/bundle.c
index 506ac49..04d62af 100644
--- a/bundle.c
+++ b/bundle.c
@@ -434,8 +434,11 @@ int create_bundle(struct bundle_header *header, const char 
*path,
init_revisions(, NULL);
 
/* write prerequisites */
-   if (compute_and_write_prerequisites(bundle_fd, , argc, argv))
+   if (compute_and_write_prerequisites(bundle_fd, , argc, argv)) {
+   if (!bundle_to_stdout)
+   close(bundle_fd);
return -1;
+   }
 
argc = setup_revisions(argc, argv, , NULL);
 
@@ -447,8 +450,11 @@ int create_bundle(struct bundle_header *header, const char 
*path,
ref_count = write_bundle_refs(bundle_fd, );
if (!ref_count)
die(_("Refusing to create empty bundle."));
-   else if (ref_count < 0)
+   else if (ref_count < 0) {
+   if (!bundle_to_stdout)
+   close(bundle_fd);
return -1;
+   }
 
/* write pack */
if (write_pack_data(bundle_fd, ))
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] notes: don't leak memory in git_config_get_notes_strategy

[Stefan Beller]

`value` is just a temporary scratchpad, so we need to make sure it doesn't
leak. It is xstrdup'd in `git_config_get_string_const` and
`parse_notes_merge_strategy` just compares the string against predefined
values, so no need to keep it around longer.

Signed-off-by: Stefan Beller 
---
 builtin/notes.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/notes.c b/builtin/notes.c
index ed6f222..d6bab17 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -751,6 +751,7 @@ static int git_config_get_notes_strategy(const char *key,
if (parse_notes_merge_strategy(value, strategy))
git_die_config(key, "unknown notes merge strategy %s", value);
 
+   free((void*)value);
return 0;
 }
 
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] path.c: allocate enough memory for string

[Stefan Beller]

`strlen` returns the length of a string without the terminating null byte.
To make sure enough memory is allocated we need to pass `strlen(..) + 1`
to the allocation function.

Signed-off-by: Stefan Beller 
---
 path.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/path.c b/path.c
index 969b494..0ae8af5 100644
--- a/path.c
+++ b/path.c
@@ -155,7 +155,7 @@ static struct trie *make_trie_node(const char *key, void 
*value)
struct trie *new_node = xcalloc(1, sizeof(*new_node));
new_node->len = strlen(key);
if (new_node->len) {
-   new_node->contents = xmalloc(new_node->len);
+   new_node->contents = xmalloc(new_node->len + 1);
memcpy(new_node->contents, key, new_node->len);
}
new_node->value = value;
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] imap-send.c, cram: allocate enough memory for null terminated string

[Stefan Beller]

`strlen` returns the length of a string without the terminating null byte.
To make sure enough memory is allocated we need to pass `strlen(..) + 1`
to the allocation function.

Signed-off-by: Stefan Beller 
---
 imap-send.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/imap-send.c b/imap-send.c
index 2c52027..f7e9909 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -872,7 +872,7 @@ static char *cram(const char *challenge_64, const char 
*user, const char *pass)
 * enough upper bound for challenge (decoded result).
 */
encoded_len = strlen(challenge_64);
-   challenge = xmalloc(encoded_len);
+   challenge = xmalloc(encoded_len + 1);
decoded_len = EVP_DecodeBlock((unsigned char *)challenge,
  (unsigned char *)challenge_64, 
encoded_len);
if (decoded_len < 0)
-- 
2.8.0.8.g27a27a6.dirty

--
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/6] Some cleanups

[Stefan Beller]

One of my first patches to Git were cleanup patches, and I fell back
to my old pattern here, while thinking on how to write better commit
messages for the submodule bugfixes I currently have in flight.

Just some one liners to not leak memory or file descriptors.

They are bundled as a series, but no patch relies on any predessor.

This applies on v2.8.0.

Thanks,
Stefan

Stefan Beller (6):
  path.c: allocate enough memory for string
  imap-send.c, cram: allocate enough memory for null terminated string
  notes: don't leak memory in git_config_get_notes_strategy
  abbrev_sha1_in_line: don't leak memory
  bundle: don't leak an fd in case of early return
  credential-cache, send_request: close fd when done

 builtin/notes.c|  1 +
 bundle.c   | 10 --
 credential-cache.c |  1 +
 imap-send.c|  2 +-
 path.c |  2 +-
 wt-status.c|  2 +-
 6 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.8.0.8.g27a27a6.dirty

--
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 v4 1/3] pretty: expand tabs in indented logs to make things line up properly

[Eric Sunshine]

On Tue, Mar 29, 2016 at 7:15 PM, Junio C Hamano  wrote:
> From: Linus Torvalds 
>
> A commit log message sometimes tries to line things up using tabs,
> assuming fixed-width font with the standard 8-place tab settings.
> Viewing such a commit however does not work well in "git log", as
> we indent the lines by prefixing 4 spaces in front of them.
> [...]
> Signed-off-by: Linus Torvalds 
> Signed-off-by: Junio C Hamano 
> ---
> diff --git a/Documentation/pretty-options.txt 
> b/Documentation/pretty-options.txt
> @@ -42,6 +42,12 @@ people using 80-column terminals.
> +--expand-tabs::
> +   Perform a tab expansion (replace each tab with enough number
> +   of spaces to fill to the next display column that is

Nit: "enough spaces" or "a sufficient number of spaces".

> +   multiple of 8) in the log message before using the message
> +   to show in the 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: weird diff output?

[Junio C Hamano]

Jacob Keller  writes:

> On Tue, Mar 29, 2016 at 11:16 AM, Stefan Beller  wrote:
>> ...
>> To find a heuristic, which appeals both the C code
>> and the shell code, we could take the empty line
>> as a strong hint for the divider:
>
> This seems like a good heuristic. Can we think of any examples where
> it would produce wildly confusing diffs? I don't think it necessarily
> needs to be default but just a possible option when formatting diffs,
> much like we already have today.

I earlier said "50% of the time it is correct, you just do not
remember", but such an option with configuration variable would let
somebody interested set it permanently for his daily use of Git, and
it would help him to find out (1) if he sees a "Huh?" division less
(or more) often than he used to, and (2) if it gives a better
division for the same change to view the diff with the plain-vanilla
heuristic.

--
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 4/6] submodule update: align reporting path for custom command execution

[Junio C Hamano]

Stefan Beller  writes:

> In the predefined actions (merge, rebase, none, checkout), we use
> the display path, which is relative to the current working directory.
> Also use the display path when running a custom command.

Very sensible.

> Signed-off-by: Stefan Beller 
> ---
>  git-submodule.sh|  4 ++--
>  t/t7406-submodule-update.sh | 29 ++---
>  2 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 11ed32a..be2a2b5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -803,8 +803,8 @@ Maybe you want to use 'update --init'?")"
>   ;;
>   !*)
>   command="${update_module#!}"
> - die_msg="$(eval_gettext "Execution of 
> '\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
> - say_msg="$(eval_gettext "Submodule path 
> '\$prefix\$sm_path': '\$command \$sha1'")"
> + die_msg="$(eval_gettext "Execution of 
> '\$command \$sha1' failed in submodule path '\$displaypath'")"
> + say_msg="$(eval_gettext "Submodule path 
> '\$displaypath': '\$command \$sha1'")"
>   must_die_on_failure=yes
>   ;;
>   *)
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 9a4ba41..f062065 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in 
> .git/config' '
>   )
>  '
>  
> +cat << EOF >expect
> +Execution of 'false $submodulesha1' failed in submodule path 'submodule'
> +EOF
> +
>  test_expect_success 'submodule update - command in .git/config catches 
> failure' '
>   (cd super &&
>git config submodule.submodule.update "!false"
>   ) &&
>   (cd super/submodule &&
> -   git reset --hard HEAD^
> +   git reset --hard $submodulesha1^
>   ) &&
>   (cd super &&
> -  test_must_fail git submodule update submodule
> - )
> +  test_must_fail git submodule update submodule 2>../actual
> + ) &&
> + test_cmp actual expect
> +'
> +
> +cat << EOF >expect
> +Execution of 'false $submodulesha1' failed in submodule path '../submodule'
> +EOF
> +
> +test_expect_success 'submodule update - command in .git/config catches 
> failure -- subdirectory' '
> + (cd super &&
> +  git config submodule.submodule.update "!false"
> + ) &&
> + (cd super/submodule &&
> +   git reset --hard $submodulesha1^
> + ) &&
> + (cd super &&
> +  mkdir tmp && cd tmp &&
> +  test_must_fail git submodule update ../submodule 2>../../actual
> + ) &&
> + test_cmp actual expect
>  '
>  
>  test_expect_success 'submodule init does not copy command into .git/config' '
--
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/6] submodule status: correct path handling in recursive submodules

[Junio C Hamano]

Stefan Beller  writes:

> The new test which is a replica of the previous test except
> that it executes from a sub directory. Prior to this patch
> the test failed by having too many '../' prefixed:
>
>   --- expect  2016-03-29 19:02:33.087336115 +
>   +++ actual  2016-03-29 19:02:33.359343311 +
>   @@ -1,7 +1,7 @@
> b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
>   -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
>   - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 
> (heads/master)
>   - 509f622a4f36a3e472affcf28fa959174f3dd5b5 
> ../nested1/nested2/nested3/submodule (heads/master)
>   ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
>   + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 
> (heads/master)
>   + 509f622a4f36a3e472affcf28fa959174f3dd5b5 
> ../../../../nested1/nested2/nested3/submodule (heads/master)
> 0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
> 0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
> 509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)
>
> The path code in question:
>   displaypath=$(relative_path "$prefix$sm_path")
>   prefix=$displaypath
>   if recursive:
> eval cmd_status
>
> That way we change `prefix` each iteration to contain another
> '../', because of the the relative_path computation is done
> on an already computed relative path.
>
> We must call relative_path exactly once with `wt_prefix` non empty.
> Further calls in recursive instances to to calculate the displaypath
> already incorporate the correct prefix from before. Fix the issue by
> clearing `wt_prefix` in recursive calls.

OK, nicely analyzed and explained.

> Signed-off-by: Stefan Beller 
> ---
>  git-submodule.sh |  1 +
>  t/t7407-submodule-foreach.sh | 21 +
>  2 files changed, 22 insertions(+)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index fdb5fbd..11ed32a 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1160,6 +1160,7 @@ cmd_status()
>   (
>   prefix="$displaypath/"
>   clear_local_git_env
> + wt_prefix=
>   cd "$sm_path" &&
>   eval cmd_status
>   ) ||

Makes sense.

Thanks.


> diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
> index 776b349..4b35e12 100755
> --- a/t/t7407-submodule-foreach.sh
> +++ b/t/t7407-submodule-foreach.sh
> @@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached 
> --recursive" preserves the --cached
>   test_cmp expect actual
>  '
>  
> +nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
> +
> +cat > expect < + $nested1sha1 ../nested1 (heads/master)
> ++$nested2sha1 ../nested1/nested2 (file2)
> + $nested3sha1 ../nested1/nested2/nested3 (heads/master)
> + $submodulesha1 ../nested1/nested2/nested3/submodule (heads/master)
> + $sub1sha1 ../sub1 ($sub1sha1_short)
> + $sub2sha1 ../sub2 ($sub2sha1_short)
> + $sub3sha1 ../sub3 (heads/master)
> +EOF
> +
> +test_expect_success 'test "status --recursive" from sub directory' '
> + (
> + cd clone3 &&
> + mkdir tmp && cd tmp &&
> + git submodule status --recursive > ../../actual
> + ) &&
> + test_cmp expect actual
> +'
> +
>  test_expect_success 'use "git clone --recursive" to checkout all submodules' 
> '
>   git clone --recursive super clone4 &&
>   (
--
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/6] submodule update --init: correct path handling in recursive submodules

[Junio C Hamano]

Stefan Beller  writes:

> The new test demonstrates a failure in the code prior to this patch.
> Instead of getting the expected
> Submodule 'submodule' (${pwd}/submodule) registered for path 
> '../super/submodule'
> the `super` directory is omitted and you get
> Submodule 'submodule' (${pwd}/submodule) registered for path 
> '../submodule'
> instead.

Same "is this about test?" comment applies here.

> That happens because the prefix is ignored in `git submodule add`, probably
> because that function itself cannot recurse;

"probably"???

> it may however called by

Probably "be" needs to be somewhere on this line.

> recursive instances of `git submodule update`, so we need to respect the
> `prefix`.
>
> Signed-off-by: Stefan Beller 
> ---
>  git-submodule.sh|  2 +-
>  t/t7406-submodule-update.sh | 33 +
>  2 files changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2838069..fdb5fbd 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -474,7 +474,7 @@ cmd_init()
>   die_if_unmatched "$mode"
>   name=$(git submodule--helper name "$sm_path") || exit
>  
> - displaypath=$(relative_path "$sm_path")
> + displaypath=$(relative_path "$prefix$sm_path")
>  
>   # Copy url setting when it is not set yet
>   if test -z "$(git config "submodule.$name.url")"
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 68ea31d..9a4ba41 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
>git submodule add ../none none &&
>test_tick &&
>git commit -m "none"
> + ) &&
> + git clone . recursivesuper &&
> + ( cd recursivesuper
> +  git submodule add ../super super
>   )
>  '
>  
> @@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
>   )
>  '
>  
> +supersha1=$(cd super && git rev-parse HEAD)

Perhaps "git -C super rev-parse HEAD"?

> +test_expect_success 'submodule update --init --recursive from subdirectory' '
> + git -C recursivesuper/super reset --hard HEAD^ &&
> + (cd recursivesuper &&
> +  mkdir tmp &&
> +  cd tmp &&
> +  git submodule update --init --recursive ../super >../../actual
> + ) &&
> + test_cmp expect actual
> +'
> +
>  apos="'";
>  test_expect_success 'submodule update does not fetch already present 
> commits' '
>   (cd 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: [PATCH 1/6] submodule foreach: correct path display in recursive submodules

[Junio C Hamano]

Stefan Beller  writes:

> The new test replicates the previous test with the difference of executing
> from a sub directory. By executing from a sub directory all we would
> expect all displayed paths to be prefixed by '../'.

-ECANTPARSE on the last sentence.  Remove the first "all" perhaps?

As this patch is no longer about a new test but is about a fix of a
problem, for which a new test serves as a typical example, it sounds
somewhat funny to start the log message with description of the test.

> Prior to this patch the test would report
> Entering 'nested1/nested2/../nested3'
> instead of the expected
> Entering '../nested1/nested2/nested3'
>
> because the prefix is put unconditionally in front and after that a
> computed display path with is affected by `wt_prefix`.

-ECANTPARSE even though I can guess what you want to say.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 43c68de..2838069 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -417,10 +417,11 @@ cmd_foreach()
>   say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
>   name=$(git submodule--helper name "$sm_path")
>   (
> - prefix="$prefix$sm_path/"
> + prefix="$(relative_path $prefix$sm_path)/"

What happens when prefix or sm_path has $IFS whitespace?  Imitate the
correct quoting you do three lines below, i.e.

prefix=$(relative_path "$prefix$sm_path")/

>   clear_local_git_env
>   cd "$sm_path" &&
>   sm_path=$(relative_path "$sm_path") &&
> + wt_prefix=
>   # we make $path available to scripts ...
>   path=$sm_path &&
>   if test $# -eq 1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GSoC] A late proposal: a modern send-email

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 29, 2016 at 6:17 AM, 惠轶群  wrote:
> 2016-03-29 0:49 GMT+08:00 Ævar Arnfjörð Bjarmason :
>> On Sat, Mar 26, 2016 at 3:13 AM, 惠轶群  wrote:
>>> 2016-03-26 2:16 GMT+08:00 Junio C Hamano :
 惠轶群  writes:

> # Purpose
> The current implementation of send-email is based on perl and has only
> a tui, it has two problems:
> - user must install a ton of dependencies before submit a single patch.
> - tui and parameter are both not quite friendly to new users.

 Is "a ton of dependencies" true?  "apt-cache show git-email"
 suggests otherwise.  Is "a ton of dependencies" truly a problem?
 "apt-get install" would resolve the dependencies for you.
>>>
>>> There are three perl packages needed to send patch through gmail:
>>> - perl-mime-tools
>>> - perl-net-smtp-ssl
>>> - perl-authen-sasl
>>>
>>> Yes, not too many, but is it better none of them?
>>>
>>> What's more, when I try to send mails, I was first disrupted by
>>> "no perl-mime-tools" then by "no perl-net-smtp-ssl or perl-authen-sasl".
>>> Then I think, why not just a mailto link?
>>
>> I think your proposal should clarify a bit who these users are that
>> find it too difficult to install these perl module dependencies. Users
>> on OSX & Windows I would assume, because in the case of Linux distros
>> getting these is the equivalent of an apt-get command away.
>
> In fact, I'm not familiar with the build for OSX or Windows.

The core of your proposal rests on the assumption that
git-send-email's implementation is problematic because it has a "ton
of dependencies", and that this must be dealt with by implementing an
alternate E-Mail transport method.

But you don't go into how this is a practical issue for users exactly,
which is the rest of the proposal. I.e. "make it friendly for users".
Let's leave the question of creating an E-Mail GUI that's shipped with
Git aside.

Correct me if I'm wrong but don't we basically have 4 kinds of users
using git-send-email:

1) Those who get it from a binary Windows package (is it even packaged there?)
2) Also a binary package, but for for OSX
3) Users installing it via their Linux distribution's package system
4) Users building it from source on Windows/OSX/Linux.

I'm in group #3 myself for the purposes of using git-send-email and
have never had issues with its dependencies because my distro's
package management takes care of it for me.

I don't know what the status is of packaging it is on #1 and #2, but
that's what I'm asking about in my question, if this becomes a
non-issue for those two groups (if it isn't already) isn't this
question of dependencies a non-issue?

I.e. why does it matter if git-send-email has N dependencies if those
N are either packaged with the common Windows/OSX packages that most
users use, or installed as dependencies by their *nix distro?

 Group #4 is small enough and likely to be a git.git contributor or
distro package maintainer anyway that this issue doesn't matter for
them.

>> If installing these dependencies is hard for users perhaps a better
>> thing to focus on is altering the binary builds on Git for platforms
>> that don't have package systems to include these dependencies.
>
> Why `mailto` not a good choice? I'm confusing.

I'm not saying having this mailto: method you're proposing isn't good
in itself, I think it would be very useful to be able to magically
open git-send-email output in your favorite E-Mail client for editing
before sending it off like you usually send E-Mail.

Although I must say I'd be seriously surprised if the likes of git
formatted patches survive contact with popular E-Mail clients when the
body is specified via the body=* parameter, given that we're sending
pretty precisely formatted content and most mailers are very eager to
wrap lines or otherwise munge input.

I'm mainly trying to get to the bottom of this dependency issue you're
trying to solve.

>> In this case it would mean shipping a statically linked OpenSSL since
>> that's what these perl SSL packages eventually depend 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

[PATCH v4 3/3] pretty: allow tweaking tabwidth in --expand-tabs

[Junio C Hamano]

When the local convention of the project is to use tab width that is
not 8, it may make sense to allow "git log --expand-tabs=" to
tweak the output to match it.

Signed-off-by: Junio C Hamano 
---
 Documentation/pretty-options.txt | 10 ++
 commit.h |  2 +-
 pretty.c | 12 ++--
 revision.c   |  9 +++--
 revision.h   |  3 +--
 5 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 23967b6..8a944b1 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,17 +42,19 @@ people using 80-column terminals.
verbatim; this means that invalid sequences in the original
commit may be copied to the output.
 
+--expand-tabs=::
 --expand-tabs::
 --no-expand-tabs::
Perform a tab expansion (replace each tab with enough number
of spaces to fill to the next display column that is
-   multiple of 8) in the log message before using the message
-   to show in the output.
+   multiple of '') in the log message before using the message
+   to show in the output.  `--expand-tabs` is a short-hand for
+   `--expand-tabs=8`, and `--no-expand-tabs` is a short-hand for
+   `--expand-tabs=0`, which disables tab expansion.
 +
 By default, tabs are expanded in pretty formats that indent the log
 message by 4 spaces (i.e.  'medium', which is the default, 'full',
-and "fuller').  `--no-expand-tabs` option can be used to disable
-this.
+and "fuller').
 
 ifndef::git-rev-list[]
 --notes[=]::
diff --git a/commit.h b/commit.h
index a7ef682..2185c8d 100644
--- a/commit.h
+++ b/commit.h
@@ -147,7 +147,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
-   unsigned expand_tabs_in_log:1;
+   unsigned expand_tabs_in_log;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
diff --git a/pretty.c b/pretty.c
index de22a8c..b340ecd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -89,11 +89,11 @@ static void setup_commit_formats(void)
 {
struct cmt_fmt_map builtin_formats[] = {
{ "raw",CMIT_FMT_RAW,   0,  0 },
-   { "medium", CMIT_FMT_MEDIUM,0,  1 },
+   { "medium", CMIT_FMT_MEDIUM,0,  8 },
{ "short",  CMIT_FMT_SHORT, 0,  0 },
{ "email",  CMIT_FMT_EMAIL, 0,  0 },
-   { "fuller", CMIT_FMT_FULLER,0,  1 },
-   { "full",   CMIT_FMT_FULL,  0,  1 },
+   { "fuller", CMIT_FMT_FULLER,0,  8 },
+   { "full",   CMIT_FMT_FULL,  0,  8 },
{ "oneline",CMIT_FMT_ONELINE,   1,  0 }
};
commit_formats_len = ARRAY_SIZE(builtin_formats);
@@ -1645,7 +1645,7 @@ static int pp_utf8_width(const char *start, const char 
*end)
return width;
 }
 
-static void strbuf_add_tabexpand(struct strbuf *sb,
+static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,
 const char *line, int linelen)
 {
const char *tab;
@@ -1666,7 +1666,7 @@ static void strbuf_add_tabexpand(struct strbuf *sb,
strbuf_add(sb, line, tab - line);
 
/* .. and the de-tabified tab */
-   strbuf_addchars(sb, ' ', 8 - (width % 8));
+   strbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));
 
/* Skip over the printed part .. */
linelen -= tab + 1 - line;
@@ -1692,7 +1692,7 @@ static void pp_handle_indent(struct pretty_print_context 
*pp,
 {
strbuf_addchars(sb, ' ', indent);
if (pp->expand_tabs_in_log)
-   strbuf_add_tabexpand(sb, line, linelen);
+   strbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen);
else
strbuf_add(sb, line, linelen);
 }
diff --git a/revision.c b/revision.c
index b1d767a..4f9ecbe 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,7 +1412,7 @@ void init_revisions(struct rev_info *revs, const char 
*prefix)
revs->skip_count = -1;
revs->max_count = -1;
revs->max_parents = -1;
-   revs->expand_tabs_in_log = 1;
+   revs->expand_tabs_in_log = 8;
 
revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1917,9 +1917,14 @@ static int handle_revision_opt(struct rev_info *revs, 
int argc, const char **arg
revs->pretty_given = 1;
get_commit_format(arg+9, revs);
} else if (!strcmp(arg, "--expand-tabs")) {
-   revs->expand_tabs_in_log = 1;
+   revs->expand_tabs_in_log = 8;
} else if (!strcmp(arg, "--no-expand-tabs")) {

[PATCH v4 0/3] Expanding tabs in "git log" output

[Junio C Hamano]

So here is the fourth try.  Previous round are at $gmane/289694,
$gmane/289166, and $gmane/288987.

I didn't quite find a good order to build this progressively, so
this series:

 - First adds the internal machinery and explicit --expand-tabs.
   This keeps Linus's authorship, but is different in that it is not
   enabled by default;

 - Then enable --expand-tabs by default for selected pretty formats;

 - And optionally, allow custom tab-width to be used.

Junio C Hamano (2):
  pretty: enable --expand-tabs by default for selected pretty formats
  pretty: allow tweaking tabwidth in --expand-tabs

Linus Torvalds (1):
  pretty: expand tabs in indented logs to make things line up properly

 Documentation/pretty-options.txt | 14 +++
 commit.h |  1 +
 log-tree.c   |  1 +
 pretty.c | 87 +++-
 revision.c   | 10 +
 revision.h   |  2 +-
 t/t4201-shortlog.sh  |  2 +-
 7 files changed, 106 insertions(+), 11 deletions(-)

-- 
2.8.0-215-gd29a7d9

--
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 v4 2/3] pretty: enable --expand-tabs by default for selected pretty formats

[Junio C Hamano]

"git log --pretty={medium,full,fuller}" and "git log" by default
prepend 4 spaces to the log message, so it makes sense to enable
the new "expand-tabs" facility by default for these formats.
Add --no-expand-tabs option to override the new default.

The change alone breaks a test in t4201 that runs "git shortlog"
on the output from "git log", and expects that the output from
"git log" does not do such a tab expansion.  Adjust the test to
explicitly disable expand-tabs with --no-expand-tabs.

Signed-off-by: Junio C Hamano 
---
 Documentation/pretty-options.txt |  6 ++
 pretty.c | 16 +---
 revision.c   |  3 +++
 t/t4201-shortlog.sh  |  2 +-
 4 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4fb5c76..23967b6 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -43,10 +43,16 @@ people using 80-column terminals.
commit may be copied to the output.
 
 --expand-tabs::
+--no-expand-tabs::
Perform a tab expansion (replace each tab with enough number
of spaces to fill to the next display column that is
multiple of 8) in the log message before using the message
to show in the output.
++
+By default, tabs are expanded in pretty formats that indent the log
+message by 4 spaces (i.e.  'medium', which is the default, 'full',
+and "fuller').  `--no-expand-tabs` option can be used to disable
+this.
 
 ifndef::git-rev-list[]
 --notes[=]::
diff --git a/pretty.c b/pretty.c
index c8b075d..de22a8c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -16,6 +16,7 @@ static struct cmt_fmt_map {
const char *name;
enum cmit_fmt format;
int is_tformat;
+   int expand_tabs_in_log;
int is_alias;
const char *user_format;
 } *commit_formats;
@@ -87,13 +88,13 @@ static int git_pretty_formats_config(const char *var, const 
char *value, void *c
 static void setup_commit_formats(void)
 {
struct cmt_fmt_map builtin_formats[] = {
-   { "raw",CMIT_FMT_RAW,   0 },
-   { "medium", CMIT_FMT_MEDIUM,0 },
-   { "short",  CMIT_FMT_SHORT, 0 },
-   { "email",  CMIT_FMT_EMAIL, 0 },
-   { "fuller", CMIT_FMT_FULLER,0 },
-   { "full",   CMIT_FMT_FULL,  0 },
-   { "oneline",CMIT_FMT_ONELINE,   1 }
+   { "raw",CMIT_FMT_RAW,   0,  0 },
+   { "medium", CMIT_FMT_MEDIUM,0,  1 },
+   { "short",  CMIT_FMT_SHORT, 0,  0 },
+   { "email",  CMIT_FMT_EMAIL, 0,  0 },
+   { "fuller", CMIT_FMT_FULLER,0,  1 },
+   { "full",   CMIT_FMT_FULL,  0,  1 },
+   { "oneline",CMIT_FMT_ONELINE,   1,  0 }
};
commit_formats_len = ARRAY_SIZE(builtin_formats);
builtin_formats_len = commit_formats_len;
@@ -172,6 +173,7 @@ void get_commit_format(const char *arg, struct rev_info 
*rev)
 
rev->commit_format = commit_format->format;
rev->use_terminator = commit_format->is_tformat;
+   rev->expand_tabs_in_log = commit_format->expand_tabs_in_log;
if (commit_format->format == CMIT_FMT_USERFORMAT) {
save_user_format(rev, commit_format->user_format,
 commit_format->is_tformat);
diff --git a/revision.c b/revision.c
index e662230..b1d767a 100644
--- a/revision.c
+++ b/revision.c
@@ -1412,6 +1412,7 @@ void init_revisions(struct rev_info *revs, const char 
*prefix)
revs->skip_count = -1;
revs->max_count = -1;
revs->max_parents = -1;
+   revs->expand_tabs_in_log = 1;
 
revs->commit_format = CMIT_FMT_DEFAULT;
 
@@ -1917,6 +1918,8 @@ static int handle_revision_opt(struct rev_info *revs, int 
argc, const char **arg
get_commit_format(arg+9, revs);
} else if (!strcmp(arg, "--expand-tabs")) {
revs->expand_tabs_in_log = 1;
+   } else if (!strcmp(arg, "--no-expand-tabs")) {
+   revs->expand_tabs_in_log = 0;
} else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) {
revs->show_notes = 1;
revs->show_notes_given = 1;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 7600a3e..2fec948 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -115,7 +115,7 @@ EOF
 '
 
 test_expect_success !MINGW 'shortlog from non-git directory' '
-   git log HEAD >log &&
+   git log --no-expand-tabs HEAD >log &&
GIT_DIR=non-existing git shortlog -w out &&
test_cmp expect out
 '
-- 
2.8.0-215-gd29a7d9

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to 

[PATCH v4 1/3] pretty: expand tabs in indented logs to make things line up properly

[Junio C Hamano]

From: Linus Torvalds 

A commit log message sometimes tries to line things up using tabs,
assuming fixed-width font with the standard 8-place tab settings.
Viewing such a commit however does not work well in "git log", as
we indent the lines by prefixing 4 spaces in front of them.

This should all line up:

  Column 1  Column 2
    
  A B
  ABCD  EFGH
  SPACESInstead of Tabs

Even with multi-byte UTF8 characters:

  Column 1  Column 2
    
  Ä B
  åäö   100
  A Møøse   once bit my sister..

Tab-expand the lines in "git log --expand-tabs" output before
prefixing 4 spaces.

This is based on the patch by Linus Torvalds, but changed to require
an explicit command line option to enable the behaviour.

Signed-off-by: Linus Torvalds 
Signed-off-by: Junio C Hamano 
---
 Documentation/pretty-options.txt |  6 
 commit.h |  1 +
 log-tree.c   |  1 +
 pretty.c | 71 ++--
 revision.c   |  2 ++
 revision.h   |  1 +
 6 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index 4b659ac..4fb5c76 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -42,6 +42,12 @@ people using 80-column terminals.
verbatim; this means that invalid sequences in the original
commit may be copied to the output.
 
+--expand-tabs::
+   Perform a tab expansion (replace each tab with enough number
+   of spaces to fill to the next display column that is
+   multiple of 8) in the log message before using the message
+   to show in the output.
+
 ifndef::git-rev-list[]
 --notes[=]::
Show the notes (see linkgit:git-notes[1]) that annotate the
diff --git a/commit.h b/commit.h
index 5d58be0..a7ef682 100644
--- a/commit.h
+++ b/commit.h
@@ -147,6 +147,7 @@ struct pretty_print_context {
int preserve_subject;
struct date_mode date_mode;
unsigned date_mode_explicit:1;
+   unsigned expand_tabs_in_log:1;
int need_8bit_cte;
char *notes_message;
struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 60f9839..78a5381 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -683,6 +683,7 @@ void show_log(struct rev_info *opt)
ctx.fmt = opt->commit_format;
ctx.mailmap = opt->mailmap;
ctx.color = opt->diffopt.use_color;
+   ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
ctx.output_encoding = get_log_output_encoding();
if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
ctx.from_ident = >from_ident;
diff --git a/pretty.c b/pretty.c
index 92b2870..c8b075d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1629,6 +1629,72 @@ void pp_title_line(struct pretty_print_context *pp,
strbuf_release();
 }
 
+static int pp_utf8_width(const char *start, const char *end)
+{
+   int width = 0;
+   size_t remain = end - start;
+
+   while (remain) {
+   int n = utf8_width(, );
+   if (n < 0 || !start)
+   return -1;
+   width += n;
+   }
+   return width;
+}
+
+static void strbuf_add_tabexpand(struct strbuf *sb,
+const char *line, int linelen)
+{
+   const char *tab;
+
+   while ((tab = memchr(line, '\t', linelen)) != NULL) {
+   int width = pp_utf8_width(line, tab);
+
+   /*
+* If it wasn't well-formed utf8, or it
+* had characters with badly defined
+* width (control characters etc), just
+* give up on trying to align things.
+*/
+   if (width < 0)
+   break;
+
+   /* Output the data .. */
+   strbuf_add(sb, line, tab - line);
+
+   /* .. and the de-tabified tab */
+   strbuf_addchars(sb, ' ', 8 - (width % 8));
+
+   /* Skip over the printed part .. */
+   linelen -= tab + 1 - line;
+   line = tab + 1;
+   }
+
+   /*
+* Print out everything after the last tab without
+* worrying about width - there's nothing more to
+* align.
+*/
+   strbuf_add(sb, line, linelen);
+}
+
+/*
+ * pp_handle_indent() prints out the intendation, and
+ * the whole line (without the final newline), after
+ * de-tabifying.
+ */
+static void pp_handle_indent(struct pretty_print_context *pp,
+struct strbuf *sb, int indent,
+const char *line, int linelen)
+{
+   strbuf_addchars(sb, ' ', indent);
+   if (pp->expand_tabs_in_log)
+   strbuf_add_tabexpand(sb, 

Re: weird diff output?

[Jacob Keller]

On Tue, Mar 29, 2016 at 11:16 AM, Stefan Beller  wrote:
> On Tue, Mar 29, 2016 at 10:54 AM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> I thought this is an optimization for C code where you have a diff like:
>>>
>>> int existingStuff1(..) {
>>> ...
>>> }
>>> +
>>> + int foo(..) {
>>> +...
>>> +}
>>>
>>> int existingStuff2(...) {
>>> ...
>>>
>>> Note that the closing '}' could be taken from the method existingStuff1 
>>> instead
>>> of correctly closing foo.
>>
>> That is a less optimal output.  Another possible output would be
>> like so:
>>
>>   int existingStuff1(..) {
>>   ...
>>   }
>>
>>  + int foo(..) {
>>  +...
>>  +}
>>  +
>>   int existingStuff2(...) {
>>
>> All three are valid output, and ...
>>
>>> So the correct heuristic really depends on what kind of text we
>>> are diffing.
>>
>> ... this realization is correct.
>>
>> I have a feeling that any heuristic would be correct half of the
>> time, including the ehuristic implemented in the current code.  The
>> readers of patches have inherent bias.  They do not notice when the
>> hunk is formed to match their expectation, but they will notice and
>> remember when they see something less optimal.
>>
>
> We have 3 possible diffs:
> 1) closing brace and newline before the chunk
> 2) newline before, closing brace after the chunk
> 3) closing brace and newline after the chunk
>
> For C code we may want to conclude that 3) is best. (appeals the bias of
> most people) 2 is slightly worse, whereas 1) is absolutely worst.
>
> Now looking at the code Jacob found strange:
>
>>  cat > expect <> + expected results ...
>> + EOF
>> +test_expect_failure  ... '
>> + ...
>> + '
>> +
>> +cat > expect <
> This can be written in two ways:
>
> 1) "cat > expect < 2) "cat > expect <
> We claim 1) is better than 2).
> This is different from the C code as now we want to have the
> same lines before not after.
>
> To find a heuristic, which appeals both the C code
> and the shell code, we could take the empty line
> as a strong hint for the divider:
>
> 1) determine the amount of diff which is ambiguous, i.e. can
>go before or after the chunk.
> 2) Does the ambiguous part contain an empty line?
> 3) If not, I have no offer for you, stop.
> 4) divide the ambiguous chunk by the empty line,
> 5) put the lines *after* the empty line in front of the chunk
> 6) put the part before (including) the empty line after the
>chunk
> 7) Observe output:
>
>>   }
>>
>>  + int foo(..) {
>>  +...
>>  +}
>>  +
>>   int existingStuff2(...) {
>
>> test_expect_failure ... '
>> existing test ...
>> '
>>
>> + cat > expect <> + expected results ...
>> + EOF
>> +test_expect_failure  ... '
>> + ...
>> + '
>> +
>> cat > expect <
> This is what we want in both cases.
> And I would argue it would appease many other kinds of text as well, because
> an empty line is usually a strong indicator for any text that a
> different thing comes along.
> (Other programming languages, such as Java, C++ and any other C like
> language behaves
> that way; even when writing latex figures you'd rather want to break
> at new lines?)
>
> Thanks,
> Stefan

This seems like a good heuristic. Can we think of any examples where
it would produce wildly confusing diffs? I don't think it necessarily
needs to be default but just a possible option when formatting diffs,
much like we already have today.

Thanks,
Jake
--
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/6] submodule update: test recursive path reporting from subdirectory

[Stefan Beller]

This patch is just a test and fixes no bug as there is currently no bug
in the path handling of `submodule update`.

In `submodule update` we make a call to `submodule--helper list --prefix
"$wt_prefix"` which looks a bit brittle and likely to introduce a bug
for the path handling. It is not a bug as the prefix is ignored inside
the submodule helper for now. If this test breaks eventually, we want
to make sure the `wt_prefix` is passed correctly into recursive submodules.
Hint: In recursive submodules we expect `wt_prefix` to be empty.

Signed-off-by: Stefan Beller 
---
 t/t7406-submodule-update.sh | 20 
 1 file changed, 20 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f062065..6dcf2d4 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -379,6 +379,26 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_cmp actual expect
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 
'../super/submodule'
+Failed to recurse into submodule path '../super'
+EOF
+
+test_expect_success 'recursive submodule update - command in .git/config 
catches failure -- subdirectory' '
+   (cd recursivesuper &&
+git submodule update --remote super &&
+git add super &&
+git commit -m "update to latest to have more than one commit in 
submodules"
+   ) &&
+   git -C recursivesuper/super config submodule.submodule.update "!false" 
&&
+   git -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&
+   (cd recursivesuper &&
+mkdir -p tmp && cd tmp &&
+test_must_fail git submodule update --recursive ../super 2>../../actual
+   ) &&
+   test_cmp actual expect
+'
+
 test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
 H=$(git ls-files -s submodule | cut -d" " -f2) &&
-- 
2.8.0.6.g3d0b0ba

--
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 6/6] t7407: make expectation as clear as possible

[Stefan Beller]

Not everyone (including me) grasps the sed expression in a split second as
they would grasp the 4 lines printed as is.

Signed-off-by: Stefan Beller 
---
 t/t7407-submodule-foreach.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 4b35e12..6ba5daf 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
 '
 
-sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" 
< expect > expect2
-mv -f expect2 expect
+cat > expect 

[PATCH 4/6] submodule update: align reporting path for custom command execution

[Stefan Beller]

In the predefined actions (merge, rebase, none, checkout), we use
the display path, which is relative to the current working directory.
Also use the display path when running a custom command.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  4 ++--
 t/t7406-submodule-update.sh | 29 ++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 11ed32a..be2a2b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -803,8 +803,8 @@ Maybe you want to use 'update --init'?")"
;;
!*)
command="${update_module#!}"
-   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
-   say_msg="$(eval_gettext "Submodule path 
'\$prefix\$sm_path': '\$command \$sha1'")"
+   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$displaypath'")"
+   say_msg="$(eval_gettext "Submodule path 
'\$displaypath': '\$command \$sha1'")"
must_die_on_failure=yes
;;
*)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9a4ba41..f062065 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in 
.git/config' '
)
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 'submodule'
+EOF
+
 test_expect_success 'submodule update - command in .git/config catches 
failure' '
(cd super &&
 git config submodule.submodule.update "!false"
) &&
(cd super/submodule &&
- git reset --hard HEAD^
+ git reset --hard $submodulesha1^
) &&
(cd super &&
-test_must_fail git submodule update submodule
-   )
+test_must_fail git submodule update submodule 2>../actual
+   ) &&
+   test_cmp actual expect
+'
+
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../submodule'
+EOF
+
+test_expect_success 'submodule update - command in .git/config catches failure 
-- subdirectory' '
+   (cd super &&
+git config submodule.submodule.update "!false"
+   ) &&
+   (cd super/submodule &&
+ git reset --hard $submodulesha1^
+   ) &&
+   (cd super &&
+mkdir tmp && cd tmp &&
+test_must_fail git submodule update ../submodule 2>../../actual
+   ) &&
+   test_cmp actual expect
 '
 
 test_expect_success 'submodule init does not copy command into .git/config' '
-- 
2.8.0.6.g3d0b0ba

--
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/6] submodule status: correct path handling in recursive submodules

[Stefan Beller]

The new test which is a replica of the previous test except
that it executes from a sub directory. Prior to this patch
the test failed by having too many '../' prefixed:

  --- expect2016-03-29 19:02:33.087336115 +
  +++ actual2016-03-29 19:02:33.359343311 +
  @@ -1,7 +1,7 @@
b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
  -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
  - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 
(heads/master)
  - 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../nested1/nested2/nested3/submodule (heads/master)
  ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
  + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 
(heads/master)
  + 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../../../../nested1/nested2/nested3/submodule (heads/master)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)

The path code in question:
  displaypath=$(relative_path "$prefix$sm_path")
  prefix=$displaypath
  if recursive:
eval cmd_status

That way we change `prefix` each iteration to contain another
'../', because of the the relative_path computation is done
on an already computed relative path.

We must call relative_path exactly once with `wt_prefix` non empty.
Further calls in recursive instances to to calculate the displaypath
already incorporate the correct prefix from before. Fix the issue by
clearing `wt_prefix` in recursive calls.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  1 +
 t/t7407-submodule-foreach.sh | 21 +
 2 files changed, 22 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index fdb5fbd..11ed32a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1160,6 +1160,7 @@ cmd_status()
(
prefix="$displaypath/"
clear_local_git_env
+   wt_prefix=
cd "$sm_path" &&
eval cmd_status
) ||
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..4b35e12 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" 
preserves the --cached
test_cmp expect actual
 '
 
+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
+
+cat > expect < ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
-- 
2.8.0.6.g3d0b0ba

--
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/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
Submodule 'submodule' (${pwd}/submodule) registered for path 
'../super/submodule'
the `super` directory is omitted and you get
Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.

That happens because the prefix is ignored in `git submodule add`, probably
because that function itself cannot recurse; it may however called by
recursive instances of `git submodule update`, so we need to respect the
`prefix`.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  2 +-
 t/t7406-submodule-update.sh | 33 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2838069..fdb5fbd 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -474,7 +474,7 @@ cmd_init()
die_if_unmatched "$mode"
name=$(git submodule--helper name "$sm_path") || exit
 
-   displaypath=$(relative_path "$sm_path")
+   displaypath=$(relative_path "$prefix$sm_path")
 
# Copy url setting when it is not set yet
if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..9a4ba41 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
 git submodule add ../none none &&
 test_tick &&
 git commit -m "none"
+   ) &&
+   git clone . recursivesuper &&
+   ( cd recursivesuper
+git submodule add ../super super
)
 '
 
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
)
 '
 
+supersha1=$(cd super && git rev-parse HEAD)
+mergingsha1=$(cd super/merging && git rev-parse HEAD)
+nonesha1=$(cd super/none && git rev-parse HEAD)
+rebasingsha1=$(cd super/rebasing && git rev-parse HEAD)
+submodulesha1=$(cd super/submodule && git rev-parse HEAD)
+pwd=$(pwd)
+
+cat ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 apos="'";
 test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
-- 
2.8.0.6.g3d0b0ba

--
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/6] submodule foreach: correct path display in recursive submodules

[Stefan Beller]

The new test replicates the previous test with the difference of executing
from a sub directory. By executing from a sub directory all we would
expect all displayed paths to be prefixed by '../'.

Prior to this patch the test would report
Entering 'nested1/nested2/../nested3'
instead of the expected
Entering '../nested1/nested2/nested3'

because the prefix is put unconditionally in front and after that a
computed display path with is affected by `wt_prefix`. This is wrong as
any relative path computation would need to be at the front. By emptying
the `wt_prefix` in recursive submodules and adding the information of any
relative path into the `prefix` this is fixed.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  3 ++-
 t/t7407-submodule-foreach.sh | 20 
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..2838069 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -417,10 +417,11 @@ cmd_foreach()
say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
name=$(git submodule--helper name "$sm_path")
(
-   prefix="$prefix$sm_path/"
+   prefix="$(relative_path $prefix$sm_path)/"
clear_local_git_env
cd "$sm_path" &&
sm_path=$(relative_path "$sm_path") &&
+   wt_prefix=
# we make $path available to scripts ...
path=$sm_path &&
if test $# -eq 1
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach 
--recursive"' '
 '
 
 cat > expect <../../actual
+   ) &&
+   test_i18ncmp expect actual
+'
+
+cat > expect 

[PATCHv3 0/6] Fix path bugs in submodule commands executed from sub dir

[Stefan Beller]

v3:
Resending without `test_pause` in the last test.

v2:
The first 3 commits are
* Test and bugfix in one commit each
* better explained than before

The expansion of an expected test result moved to the back of the series.

There are two new commits
* one being another bugfix of the display path for `submodule update`
* another test for `submodule update` as I suspect it may break further on
  refactoring and we currently have no tests for it.

Thanks,
Stefan

Stefan Beller (6):
  submodule foreach: correct path display in recursive submodules
  submodule update --init: correct path handling in recursive submodules
  submodule status: correct path handling in recursive submodules
  t7407: make expectation as clear as possible
  submodule update: align reporting path for custom command execution
  submodule update: test recursive path reporting from subdirectory

 git-submodule.sh | 10 +++---
 t/t7406-submodule-update.sh  | 83 ++--
 t/t7407-submodule-foreach.sh | 49 --
 3 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.8.0.4.g5639dee.dirty

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

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

[Junio C Hamano]

Jeff King  writes:

>   $ echo content >../file
>   $ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git add file
>   fatal: pathspec 'file' did not match any files
>
> I'd expect that to work, and it doesn't, because we pass ".git/" as the
> "prefix" to cmd_add(). Which I guess is true, but it feels kind of weird
> (I think most people who set both variables like that would generally
> point to some other directory entirely, and we would have a NULL
> prefix).

That reminds me of a related tangent.  If we really want to properly
support running from outside the working tree (or from inside .git
for that matter), I suspect we need two separate "prefix" for two
different uses.  The "we would have a NULL prefix" is what was
considered the true "prefix" traditionally, i.e. it is the directory
to which any pathspecs and relative paths that name paths in the
history are taken relative to.  E.g. if you run "git add HEAD" from
inside your GIT_DIR but you have GIT_WORK_TREE set up correctly, you
would want to add HEAD from the root of the working tree.

Another is the base directory for a relative filename that names a
file that does not have anything to do with the paths in the
history.  E.g. if you run "git grep --file patterns" from outside
the working tree but with GIT_DIR/GIT_WORK_TREE correctly set up,
you would still want to read the "patterns" file from the current
directory.

The former can be done by using prefix=NULL to say "we may or may
not have come from outside a working tree but we no longer care
after we chdir(2) to the root of the working tree.  Any path is
relative to the root of the working tree."  But then we may lose the
clue to read from the latter (the OPT_FILENAME option is handled by
prefix_filename() using the prefix).

The distinction between the two does not exist as long as you start
inside GIT_WORK_TREE and outside GIT_DIR.

--
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: [PATCHv2 0/6] Fix path bugs in submodule commands executed from sub dir

[Stefan Beller]

Please ignore this series; it was sent out prematurely. :(
--
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 6/6] submodule update: test recursive path reporting from subdirectory

[Stefan Beller]

This patch is just a test and fixes no bug as there is currently no bug
in the path handling of `submodule update`.

In `submodule update` we make a call to `submodule--helper list --prefix
"$wt_prefix"` which looks a bit brittle and likely to introduce a bug
for the path handling. It is not a bug as the prefix is ignored inside
the submodule helper for now. If this test breaks eventually, we want
to make sure the `wt_prefix` is passed correctly into recursive submodules.
Hint: In recursive submodules we expect `wt_prefix` to be empty.

Signed-off-by: Stefan Beller 
---
 t/t7406-submodule-update.sh | 21 +
 1 file changed, 21 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index f062065..239d31f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -379,6 +379,27 @@ test_expect_success 'submodule update - command in 
.git/config catches failure -
test_cmp actual expect
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 
'../super/submodule'
+Failed to recurse into submodule path '../super'
+EOF
+
+test_expect_success 'recursive submodule update - command in .git/config 
catches failure -- subdirectory' '
+   (cd recursivesuper &&
+git submodule update --remote super &&
+git add super &&
+git commit -m "update to latest to have more than one commit in 
submodules"
+   ) &&
+   git -C recursivesuper/super config submodule.submodule.update "!false" 
&&
+   git -C recursivesuper/super/submodule reset --hard $submodulesha1^ &&
+   (cd recursivesuper &&
+mkdir -p tmp && cd tmp &&
+test_pause &&
+test_must_fail git submodule update --recursive ../super 2>../../actual
+   ) &&
+   test_cmp actual expect
+'
+
 test_expect_success 'submodule init does not copy command into .git/config' '
(cd super &&
 H=$(git ls-files -s submodule | cut -d" " -f2) &&
-- 
2.8.0.4.g5639dee.dirty

--
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/6] submodule status: correct path handling in recursive submodules

[Stefan Beller]

The new test which is a replica of the previous test except
that it executes from a sub directory. Prior to this patch
the test failed by having too many '../' prefixed:

  --- expect2016-03-29 19:02:33.087336115 +
  +++ actual2016-03-29 19:02:33.359343311 +
  @@ -1,7 +1,7 @@
b23f134787d96fae589a6b76da41f4db112fc8db ../nested1 (heads/master)
  -+25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../nested1/nested2 (file2)
  - 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../nested1/nested2/nested3 
(heads/master)
  - 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../nested1/nested2/nested3/submodule (heads/master)
  ++25d56d1ddfb35c3e91ff7d8f12331c2e53147dcc ../../nested1/nested2 (file2)
  + 5ec83512b76a0b8170b899f8e643913c3e9b72d9 ../../../nested1/nested2/nested3 
(heads/master)
  + 509f622a4f36a3e472affcf28fa959174f3dd5b5 
../../../../nested1/nested2/nested3/submodule (heads/master)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub1 (0c90624)
0c90624ab7f1aaa301d3bb79f60dcfed1ec4897f ../sub2 (0c90624)
509f622a4f36a3e472affcf28fa959174f3dd5b5 ../sub3 (heads/master)

The path code in question:
  displaypath=$(relative_path "$prefix$sm_path")
  prefix=$displaypath
  if recursive:
eval cmd_status

That way we change `prefix` each iteration to contain another
'../', because of the the relative_path computation is done
on an already computed relative path.

We must call relative_path exactly once with `wt_prefix` non empty.
Further calls in recursive instances to to calculate the displaypath
already incorporate the correct prefix from before. Fix the issue by
clearing `wt_prefix` in recursive calls.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  1 +
 t/t7407-submodule-foreach.sh | 21 +
 2 files changed, 22 insertions(+)

diff --git a/git-submodule.sh b/git-submodule.sh
index fdb5fbd..11ed32a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1160,6 +1160,7 @@ cmd_status()
(
prefix="$displaypath/"
clear_local_git_env
+   wt_prefix=
cd "$sm_path" &&
eval cmd_status
) ||
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 776b349..4b35e12 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -277,6 +277,27 @@ test_expect_success 'ensure "status --cached --recursive" 
preserves the --cached
test_cmp expect actual
 '
 
+nested2sha1=$(git -C clone3/nested1/nested2 rev-parse HEAD)
+
+cat > expect < ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 test_expect_success 'use "git clone --recursive" to checkout all submodules' '
git clone --recursive super clone4 &&
(
-- 
2.8.0.4.g5639dee.dirty

--
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/6] submodule update: align reporting path for custom command execution

[Stefan Beller]

In the predefined actions (merge, rebase, none, checkout), we use
the display path, which is relative to the current working directory.
Also use the display path when running a custom command.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  4 ++--
 t/t7406-submodule-update.sh | 29 ++---
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 11ed32a..be2a2b5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -803,8 +803,8 @@ Maybe you want to use 'update --init'?")"
;;
!*)
command="${update_module#!}"
-   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$prefix\$sm_path'")"
-   say_msg="$(eval_gettext "Submodule path 
'\$prefix\$sm_path': '\$command \$sha1'")"
+   die_msg="$(eval_gettext "Execution of 
'\$command \$sha1' failed in submodule path '\$displaypath'")"
+   say_msg="$(eval_gettext "Submodule path 
'\$displaypath': '\$command \$sha1'")"
must_die_on_failure=yes
;;
*)
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 9a4ba41..f062065 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -344,16 +344,39 @@ test_expect_success 'submodule update - command in 
.git/config' '
)
 '
 
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path 'submodule'
+EOF
+
 test_expect_success 'submodule update - command in .git/config catches 
failure' '
(cd super &&
 git config submodule.submodule.update "!false"
) &&
(cd super/submodule &&
- git reset --hard HEAD^
+ git reset --hard $submodulesha1^
) &&
(cd super &&
-test_must_fail git submodule update submodule
-   )
+test_must_fail git submodule update submodule 2>../actual
+   ) &&
+   test_cmp actual expect
+'
+
+cat << EOF >expect
+Execution of 'false $submodulesha1' failed in submodule path '../submodule'
+EOF
+
+test_expect_success 'submodule update - command in .git/config catches failure 
-- subdirectory' '
+   (cd super &&
+git config submodule.submodule.update "!false"
+   ) &&
+   (cd super/submodule &&
+ git reset --hard $submodulesha1^
+   ) &&
+   (cd super &&
+mkdir tmp && cd tmp &&
+test_must_fail git submodule update ../submodule 2>../../actual
+   ) &&
+   test_cmp actual expect
 '
 
 test_expect_success 'submodule init does not copy command into .git/config' '
-- 
2.8.0.4.g5639dee.dirty

--
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/6] t7407: make expectation as clear as possible

[Stefan Beller]

Not everyone (including me) grasps the sed expression in a split second as
they would grasp the 4 lines printed as is.

Signed-off-by: Stefan Beller 
---
 t/t7407-submodule-foreach.sh | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 4b35e12..6ba5daf 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -262,8 +262,12 @@ test_expect_success 'test "status --recursive"' '
test_cmp expect actual
 '
 
-sed -e "/nested2 /s/.*/+$nested2sha1 nested1\/nested2 (file2~1)/;/sub[1-3]/d" 
< expect > expect2
-mv -f expect2 expect
+cat > expect 

[PATCH 1/6] submodule foreach: correct path display in recursive submodules

[Stefan Beller]

The new test replicates the previous test with the difference of executing
from a sub directory. By executing from a sub directory all we would
expect all displayed paths to be prefixed by '../'.

Prior to this patch the test would report
Entering 'nested1/nested2/../nested3'
instead of the expected
Entering '../nested1/nested2/nested3'

because the prefix is put unconditionally in front and after that a
computed display path with is affected by `wt_prefix`. This is wrong as
any relative path computation would need to be at the front. By emptying
the `wt_prefix` in recursive submodules and adding the information of any
relative path into the `prefix` this is fixed.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh |  3 ++-
 t/t7407-submodule-foreach.sh | 20 
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 43c68de..2838069 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -417,10 +417,11 @@ cmd_foreach()
say "$(eval_gettext "Entering '\$prefix\$displaypath'")"
name=$(git submodule--helper name "$sm_path")
(
-   prefix="$prefix$sm_path/"
+   prefix="$(relative_path $prefix$sm_path)/"
clear_local_git_env
cd "$sm_path" &&
sm_path=$(relative_path "$sm_path") &&
+   wt_prefix=
# we make $path available to scripts ...
path=$sm_path &&
if test $# -eq 1
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 7ca10b8..776b349 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -178,6 +178,26 @@ test_expect_success 'test messages from "foreach 
--recursive"' '
 '
 
 cat > expect <../../actual
+   ) &&
+   test_i18ncmp expect actual
+'
+
+cat > expect 

[PATCHv2 0/6] Fix path bugs in submodule commands executed from sub dir

[Stefan Beller]

The first 3 commits are
* Test and bugfix in one commit each
* better explained than before

The expansion of an expected test result moved to the back of the series.

There are two new commits
* one being another bugfix of the display path for `submodule update`
* another test for `submodule update` as I suspect it may break further on
  refactoring and we currently have no tests for it.

Thanks,
Stefan

Stefan Beller (6):
  submodule foreach: correct path display in recursive submodules
  submodule update --init: correct path handling in recursive submodules
  submodule status: correct path handling in recursive submodules
  t7407: make expectation as clear as possible
  submodule update: align reporting path for custom command execution
  submodule update: test recursive path reporting from subdirectory

 git-submodule.sh | 10 +++---
 t/t7406-submodule-update.sh  | 83 ++--
 t/t7407-submodule-foreach.sh | 49 --
 3 files changed, 133 insertions(+), 9 deletions(-)

-- 
2.8.0.4.g5639dee.dirty

--
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/6] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

The new test demonstrates a failure in the code prior to this patch.
Instead of getting the expected
Submodule 'submodule' (${pwd}/submodule) registered for path 
'../super/submodule'
the `super` directory is omitted and you get
Submodule 'submodule' (${pwd}/submodule) registered for path '../submodule'
instead.

That happens because the prefix is ignored in `git submodule add`, probably
because that function itself cannot recurse; it may however called by
recursive instances of `git submodule update`, so we need to respect the
`prefix`.

Signed-off-by: Stefan Beller 
---
 git-submodule.sh|  2 +-
 t/t7406-submodule-update.sh | 33 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index 2838069..fdb5fbd 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -474,7 +474,7 @@ cmd_init()
die_if_unmatched "$mode"
name=$(git submodule--helper name "$sm_path") || exit
 
-   displaypath=$(relative_path "$sm_path")
+   displaypath=$(relative_path "$prefix$sm_path")
 
# Copy url setting when it is not set yet
if test -z "$(git config "submodule.$name.url")"
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 68ea31d..9a4ba41 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -63,6 +63,10 @@ test_expect_success 'setup a submodule tree' '
 git submodule add ../none none &&
 test_tick &&
 git commit -m "none"
+   ) &&
+   git clone . recursivesuper &&
+   ( cd recursivesuper
+git submodule add ../super super
)
 '
 
@@ -95,6 +99,35 @@ test_expect_success 'submodule update from subdirectory' '
)
 '
 
+supersha1=$(cd super && git rev-parse HEAD)
+mergingsha1=$(cd super/merging && git rev-parse HEAD)
+nonesha1=$(cd super/none && git rev-parse HEAD)
+rebasingsha1=$(cd super/rebasing && git rev-parse HEAD)
+submodulesha1=$(cd super/submodule && git rev-parse HEAD)
+pwd=$(pwd)
+
+cat ../../actual
+   ) &&
+   test_cmp expect actual
+'
+
 apos="'";
 test_expect_success 'submodule update does not fetch already present commits' '
(cd submodule &&
-- 
2.8.0.4.g5639dee.dirty

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

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

[Jeff King]

On Tue, Mar 29, 2016 at 11:00:03PM +0100, John Keeping wrote:

> > We seem to get that wrong. I'm also not sure if it would make sense if
> > you explicitly set the two to be equal, like:
> > 
> >   # checking in your own refs?
> >   GIT_WORK_TREE=$(pwd) GIT_DIR=$(pwd) git add refs packed-refs
> > 
> > So the current behavior may just be weird-but-true.
> 
> This case definitely feels wrong:
> 
>   $ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git rev-parse 
> --is-inside-git-dir
>   false

Yeah, and not just the is-inside-git-dir test:

  $ echo content >../file
  $ GIT_WORK_TREE=$(cd ..; pwd) GIT_DIR=$(pwd) git add file
  fatal: pathspec 'file' did not match any files

I'd expect that to work, and it doesn't, because we pass ".git/" as the
"prefix" to cmd_add(). Which I guess is true, but it feels kind of weird
(I think most people who set both variables like that would generally
point to some other directory entirely, and we would have a NULL
prefix).

The --is-inside-git-dir thing is related, but a different problem. I
just got your follow-up mentioning that it doesn't take the prefix into
account, which I agree it probably should.

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

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

[John Keeping]

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

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

real_path(startup_info->prefix)

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

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

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

[John Keeping]

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

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

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

This case definitely feels wrong:

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

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

If GIT_WORK_TREE points somewhere unrelated then it is correct:

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

Re: [PATCH v2 03/17] index-helper: new daemon for caching index and related stuff

[David Turner]

On Tue, 2016-03-29 at 09:31 +0700, Duy Nguyen wrote:
> On Sat, Mar 19, 2016 at 8:04 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > From: Nguyễn Thái Ngọc Duy 
> > 
> > Instead of reading the index from disk and worrying about disk
> > corruption, the index is cached in memory (memory bit-flips happen
> > too, but hopefully less often). The result is faster read. Read
> > time
> > is reduced by 70%.
> > 
> > The biggest gain is not having to verify the trailing SHA-1, which
> > takes lots of time especially on large index files. But this also
> > opens doors for further optimiztions:
> > 
> >  - we could create an in-memory format that's essentially the
> > memory
> >dump of the index to eliminate most of parsing/allocation
> >overhead. The mmap'd memory can be used straight away.
> > Experiment
> >[1] shows we could reduce read time by 88%.
> 
> This reference [1] is missing (even in my old version). I believe
> it's
> http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=2
> 48771,
> comparing 256.442ms in that mail with v4 number, 2245.113ms in 0/8
> mail from the same thread.
> 
> > Git can poke the daemon via named pipes to tell it to refresh the
> > index cache, or to keep it alive some more minutes. It can't give
> > any
> > real index data directly to the daemon. Real data goes to disk
> > first,
> > then the daemon reads and verifies it from there. Poking only
> > happens
> > for $GIT_DIR/index, not temporary index files.
> 
> I think we should go with unix socket on *nix platform instead of
> named pipe. UNIX named pipe only allows one communication channel at
> a
> time. Windows named pipe is different and allows multiple clients,
> which is the same as unix socket.
> 
> 
> > $GIT_DIR/index-helper.pipe is the named pipe for daemon process.
> > The
> > daemon reads from the pipe and executes commands.  Commands that
> > need
> > replies from the daemon will have to open their own pipe, since a
> > named pipe should only have one reader.  Unix domain sockets don't
> > have this problem, but are less portable.
> 
> Hm..NO_UNIX_SOCKETS is only set for Windows in config.mak.uname and
> Windows will need to be specially tailored anyway, I think unix
> socket
> would be more elegant.

One annoyance with unix sockets is that they must have short paths
(UNIX_PATH_MAX -- about a hundred characters).  This basically means
they should be in $TMPDIR.  I guess we could go back to pid files in
$GIT_DIR, and then have a socket named after the pid.  There's also
some security issues, but it actually looks like there's a simple
enough workaround for them.

I'll change this, but it might take a bit as I'm busy with other things
this week.

> > +static void share_index(struct index_state *istate, struct shm
> > *is)
> > +{
> > +   void *new_mmap;
> > +   if (istate->mmap_size <= 20 ||
> > +   hashcmp(istate->sha1,
> > +   (unsigned char *)istate->mmap + istate
> > ->mmap_size - 20) ||
> > +   !hashcmp(istate->sha1, is->sha1) ||
> > +   git_shm_map(O_CREAT | O_EXCL | O_RDWR, 0700, istate
> > ->mmap_size,
> > +   _mmap, PROT_READ | PROT_WRITE,
> > MAP_SHARED,
> > +   "git-index-%s", sha1_to_hex(istate->sha1))
> > < 0)
> > +   return;
> > +
> > +   release_index_shm(is);
> > +   is->size = istate->mmap_size;
> > +   is->shm = new_mmap;
> > +   hashcpy(is->sha1, istate->sha1);
> > +   memcpy(new_mmap, istate->mmap, istate->mmap_size - 20);
> > +
> > +   /*
> > +* The trailing hash must be written last after everything
> > is
> > +* written. It's the indication that the shared memory is
> > now
> > +* ready.
> > +*/
> > +   hashcpy((unsigned char *)new_mmap + istate->mmap_size - 20,
> > is->sha1);
> 
> You commented here [1] a long time ago about memory barrier. I'm not
> entirely sure if compilers dare to reorder function calls, but when
> hashcpy is inlined and memcpy is builtin, I suppose that's
> possible...
> 
> [1] http://article.gmane.org/gmane.comp.version-control.git/280729

Oh, right.  Will fix.
--
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 00/19] index-helper, watchman

[David Turner]

On Tue, 2016-03-29 at 19:09 +0200, Torsten Bögershausen wrote:
> On 2016-03-09 19.36, David Turner wrote:
> > This is a rebase and extension of Duy's work on git index-helper
> > and
> > watchman support.
> > 
> Somewhere we need to tweak something:
> t7900 do all fail under Mac OS, because the index-helper is not
> build.
> 
> The best would be to have a precondition when running the tests ?
> 
> t7900-index-helper.sh   not ok 1 - index-helper smoke test
> t7900-index-helper.sh   not ok 2 - index-helper creates usable pipe
> file and can
> be killed
> t7900-index-helper.sh   not ok 3 - index-helper will not start if
> already running
> t7900-index-helper.sh   not ok 4 - index-helper is quiet with -
> -autorun
> t7900-index-helper.sh   not ok 5 - index-helper autorun works
> 
> 
> The other thing is to enable SHM on other platforms, but first things
> first.

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

Re: [PATCH v2 08/17] read-cache: invalidate untracked cache data when reading WAMA

[David Turner]

On Tue, 2016-03-29 at 09:50 +0700, Duy Nguyen wrote:
> On Sat, Mar 19, 2016 at 8:04 AM, David Turner <
> dtur...@twopensource.com> wrote:
> > @@ -1407,10 +1472,24 @@ static int read_watchman_ext(struct
> > index_state *istate, const void *data,
> > ewah_each_bit(bitmap, mark_no_watchman, istate);
> > ewah_free(bitmap);
> > 
> > -   /*
> > -* TODO: update the untracked cache from the untracked data
> > in this
> > -* extension.
> > -*/
> > +   if (istate->untracked && istate->untracked->root) {
> > +   int i;
> > +   const char *untracked;
> > +
> > +   untracked = data + len + 8 + bitmap_size;
> > +   for (i = 0; i < untracked_nr; ++i) {
> > +   int len = strlen(untracked);
> > +   string_list_append(>untracked
> > ->invalid_untracked,
> > +  untracked);
> > +   untracked += len + 1;
> > +   }
> > +
> > +   for_each_string_list(>untracked
> > ->invalid_untracked,
> > +mark_untracked_invalid, istate
> > ->untracked);
> 
> I think it's a bit early to invalidate untracked cache here. We can
> do
> that in refresh_by_watchman() in 10/17, where ce_mark_uptodate() to
> prevent lstat() is also done.

Will move/squash

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

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

[Jeff King]

On Tue, Mar 29, 2016 at 09:52:08PM +0100, John Keeping wrote:

> > Yeah, I think this is a bug. Presumably what is happening is that we are
> > too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
> > we ask "are we in a work tree", the answer has become yes. But the
> > caller really wants to know "am _I_ inside the work tree".
> 
> I don't think that's what's happening.  Try:
> 
>   $ cd .git/
>   $ GIT_WORK_TREE=.. git rev-parse --is-inside-work-tree
>   true
> 
> so I think it's that we refuse to assume that the directory above a Git
> directory is a working tree (something similar happens when the
> "core.worktree" config variable is set).  I'm not convinced that's
> unreasonable.

Yeah, you're right, but I'm not sure how your example shows that, (isn't
it basically the same as Elliott's original, except using a relative
path?). A more compelling counter-example to my hypothesis is:

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

So it is not that we chdir too early, but just that we blindly check "is
$(pwd) inside $GIT_WORK_TREE". And it does not create a problem for the
normal discovered-path cases, because either:

  - we discovered .git by walking up the directory tree, which means we
must be in a work-tree

  - we discovered that we are inside a .git directory, and therefore
take it to be bare (and thus there is no work tree, and we cannot be
inside it). This is what happens in Elliott's original example that
behaves differently than the $GIT_WORK_TREE case.

I'd be tempted to say that "inside the work tree" is further clarified
to "not inside the $GIT_DIR". But as you note:

> However, the case above also gives:
> 
>   $ GIT_WORK_TREE=.. git rev-parse --is-inside-git-dir
>   false
>   $ test $(pwd) = $(GIT_WORK_TREE=.. git rev-parse --git-dir); echo $?
>   0
> 
> so even though $PWD *is* the Git directory, we're not in the Git
> directory!  Setting GIT_DIR=$(pwd) makes no different to that.

We seem to get that wrong. I'm also not sure if it would make sense if
you explicitly set the two to be equal, like:

  # checking in your own refs?
  GIT_WORK_TREE=$(pwd) GIT_DIR=$(pwd) git add refs packed-refs

So the current behavior may just be weird-but-true.

-Peff
--
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] t/t5520: test --[no-]autostash with pull.rebase=true

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:30 AM, Mehul Jain  wrote:
> "--[no-]autostash" option for git-pull is only valid in rebase mode.
> That is, either --rebase is used or pull.rebase=true. Existing tests
> already check the cases when --rebase is used but fails to check for
> pull.rebase=true case.
>
> Add two new tests to check that --[no-]autostash option works with
> pull.rebase=true.
>
> Signed-off-by: Mehul Jain 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -316,6 +316,26 @@ test_expect_success 'pull.rebase' '
> +test_expect_success 'pull --autostash & pull.rebase=true' '
> +   test_config pull.rebase true &&
> +   git reset --hard before-rebase &&
> +   echo dirty >new_file &&
> +   git add new_file &&
> +   git pull --autostash . copy &&
> +   test_cmp_rev HEAD^ copy &&
> +   test "$(cat new_file)" = dirty &&
> +   test "$(cat file)" = "modified again"
> +'

With the exception of the missing --rebase argument, this is exactly
the same code as in test_rebase_autostash(), right? Rather than
repeating this code yet again, it might be nice to augment that
function to accept a (possibly) optional argument controlling whether
--rebase is used.

> +
> +test_expect_success 'pull --no-autostash & pull.rebase=true' '
> +   test_config pull.rebase true &&
> +   git reset --hard before-rebase &&
> +   echo dirty >new_file &&
> +   git add new_file &&
> +   test_must_fail git pull --no-autostash . copy 2>err &&
> +   test_i18ngrep "Cannot pull with rebase: Your index contains 
> uncommitted changes." err
> +'

Ditto with regard to test_rebase_no_autostash() (or
test_rebase_autostash_fail() as I suggested in my patch 4/5 review).
--
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 4/5] t/t5520: modify tests to reduce common code

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain  wrote:
> t/t5520: modify tests to reduce common code

As this is indeed a patch, "modify" is implied. Perhaps:

t5520: factor out common code

> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.

This patch is doing so much that it's difficult to review for
correctness. Taking [1] into consideration, better would be to split
it into at least three patches:

1. Factor out code into test_rebase_autostash() and modify the four
tests to call it.

2. Factor out code into test_rebase_autostash_fail() and modify the
three tests to call it.

3. Fold the two "pull $i (without --rebase) is illegal" tests into a for-loop.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/289434/focus=289860

> Helped-by: Eric Sunshine 
> Signed-off-by: Mehul Jain 
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -9,6 +9,24 @@ modify () {
> +test_rebase_no_autostash () {
> +   git reset --hard before-rebase &&
> +   echo dirty >new_file &&
> +   git add new_file &&
> +   test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> +   test_i18ngrep "Cannot pull with rebase: Your index contains 
> uncommitted changes." err

In the spirit of patch 3/5 and [1], you could grep for a substring
rather than the full message, but that's a minor point, not worth a
re-roll.

test_i18ngrep "uncommitted changes" err

> +}
> @@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty 
> working directory and reb
> +for i in true false
> +   do
> +   test_expect_success "pull --rebase --autostash & 
> rebase.autostash=$i" '
> +   test_config rebase.autostash $i &&
> +   test_rebase_autostash
> +   '
> +   done

I don't care too strongly, but I'm not convinced that this for-loop is
buying you much for these two cases since each test already has been
reduced to two simple lines, and the added abstraction of the for-loop
increases cognitive load a bit.

> +for i in --autostash --no-autostash
> +   do
> +   test_expect_success "pull $i (without --rebase) is illegal" '
> +   test_must_fail git pull $i . copy 2>actual &&
> +   test_i18ngrep "only valid with --rebase" actual
> +   '
> +   done

You might then ask why I suggested[1] the for-loop in this case but
not for the true/false case. Even though these are also two-line
tests, they are not quite as simple as two lines down to which the
true/false tests devolve. Anyhow, this alone is not worth a re-roll.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[John Keeping]

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

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

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

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

However, the case above also gives:

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

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

Re: [PATCH v1 6/7] correct blame for files commited with CRLF

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> Torsten Bögershausen  writes:
>>
>>> If we had made the CRLF -> LF conversion, yes. But we don't do that.
>>> crlf_to_git() returns without touching the line endings.
>>
>> That sounds quite broken.  How would a user ever fix broken data in
>> the index then?  I know the commit that often appears in these
>> discussions claims to give us "safer CRLF" handling, but I have a
>> suspicion that perhaps we should rethink if that claim is really
>> true.  Isn't it giving us more problems than it is worth?
>
> Having said all that, within the context of the current codebase
> where autocrlf refuses to do the conversion, I agree that teaching
> blame to follow the same logic makes sense.  Let me review the
> series up to 6/7 with fresh eyes.

And for the same reason 7/7 may make sense (I didn't check the
implementation, but I think I understand your motivation well enough
to comment)--if crlf-to-git returns without actually converting upon
next "git add $path", in order to serve a preview of what change you
would be checking in and/or committing when you do "git add $path"
and/or "git commit $path", the _to_git() conversion "git diff" and
"git diff HEAD" do on the contents taken from the working tree
should follow the same logic.

"git diff $tree-ish" (of which "git diff HEAD" is a special case) is
a bit tricky to reason about, but I think using the "does index have
CR to cause us refuse conversion?" logic is a sensible thing to do
even in that case.  It is asking what difference you would have if
you committed the state in the working tree right now, and the "does
index have CR?" logic to kick in, even though the contents of the
index may not be something that was derived from the unrelated
$tree-ish, would kick in when you make the hypothetical commit to be
compared against $tree-ish.

Again, let me review 7/7 as well with fresh eyes.

Thanks.

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

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

[Jeff King]

On Tue, Mar 29, 2016 at 06:42:44AM -0500, Elliott Cable wrote:

> So, I find this behaviour a little strange; I can't determine if it's
> a subtle bug, or intentionally undefined/‘fuzzy’ behaviour:
> 
> $ cd a-repo/.git/
> $ pwd
> /path/to/a-repo/.git
> $ git rev-parse --is-inside-work-tree
> false
> $ export GIT_WORK_TREE=/path/to/a-repo
> $ git rev-parse --is-inside-work-tree
> true
> 
> i.e. when within the repository (the `.git` directory), and when that
> directory is a sub-directory of the working-tree, `rev-parse
> --is-inside-work-tree` reports *false* (reasonable enough, I suppose);
> but then if `$GIT_WORK_TREE` is set to precisely the directory that
> git was *already* assuming was the working-directory, then the same
> command, in the same location, reports *true*.
> 
> This should probably be made consistent: either `rev-parse
> --is-inside-work-tree` should report “true”, even inside the `.git`
> dir, as long as that directory is a sub-directory of the working-tree
> … or repository-directories / `$GIT_DIR` / `.git` directories should
> be excluded from truthy responses to `rev-parse
> --is-inside-work-tree`.

Yeah, I think this is a bug. Presumably what is happening is that we are
too eager to "cd $GIT_WORK_TREE" inside git-rev-parse, and by the time
we ask "are we in a work tree", the answer has become yes. But the
caller really wants to know "am _I_ inside the work tree".

Unfortunately, I think the fix is likely to be rather tricky, as the
work-tree stuff is happening deep inside setup_git_directory().

-Peff
--
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 v1 6/7] correct blame for files commited with CRLF

[Junio C Hamano]

Junio C Hamano  writes:

> Torsten Bögershausen  writes:
>
>> If we had made the CRLF -> LF conversion, yes. But we don't do that.
>> crlf_to_git() returns without touching the line endings.
>
> That sounds quite broken.  How would a user ever fix broken data in
> the index then?  I know the commit that often appears in these
> discussions claims to give us "safer CRLF" handling, but I have a
> suspicion that perhaps we should rethink if that claim is really
> true.  Isn't it giving us more problems than it is worth?

Having said all that, within the context of the current codebase
where autocrlf refuses to do the conversion, I agree that teaching
blame to follow the same logic makes sense.  Let me review the
series up to 6/7 with fresh eyes.

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

Re: [PATCH 3/5] t/t5520: use test_i18ngrep instead of test_cmp

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain  wrote:
> t/t5520: use test_i18ngrep instead of test_cmp

As mentioned for earlier patches, this is too low-level, whereas it
should be giving a high-level overview.

> test_cmp is used for error checking when test_i18ngrep could be used.
>
> Use test_i18ngrep to check for the valid error.

"could be used" is not sufficient justification to explain why this
change is desirable. See [1] for a good explanation of why this change
should be made.

The patch itself looks fine.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289077

> Signed-off-by: Mehul Jain 
> ---
>  t/t5520-pull.sh | 10 --
>  1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9ee2218..d03cb84 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -317,15 +317,13 @@ test_expect_success 'pull --rebase --no-autostash & 
> rebase.autostash unset' '
>  '
>
>  test_expect_success 'pull --autostash (without --rebase) should error out' '
> -   test_must_fail git pull --autostash . copy 2>actual &&
> -   echo "fatal: --[no-]autostash option is only valid with --rebase." 
> >expect &&
> -   test_i18ncmp actual expect
> +   test_must_fail git pull --autostash . copy 2>err &&
> +   test_i18ngrep "only valid with --rebase" err
>  '
>
>  test_expect_success 'pull --no-autostash (without --rebase) should error 
> out' '
> -   test_must_fail git pull --no-autostash . copy 2>actual &&
> -   echo "fatal: --[no-]autostash option is only valid with --rebase." 
> >expect &&
> -   test_i18ncmp actual expect
> +   test_must_fail git pull --no-autostash . copy 2>err &&
> +   test_i18ngrep "only valid with --rebase" err
>  '
>
>  test_expect_success 'pull.rebase' '
> --
> 2.7.1.340.g69eb491.dirty
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

John Keeping  writes:

> Having gone looking, I can't find a reference but I for some reason I
> was convinced the separate version was preferred in the option
> descriptions. 

The one that is similar that came back to my mind while reading your
response was that we used to have these:

--foo, --bar::
Description...

and corrected them to:

--foo::
--bar::
Description...


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

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

[Jeff King]

On Tue, Mar 29, 2016 at 12:56:41PM -0700, Junio C Hamano wrote:

> >> So it is a misconfiguration if you only set GIT_WORK_TREE without
> >> setting GIT_DIR.
> >
> > Hmm. I have frequently done this when my cwd is a git repository (e.g.,
> > a bare one), and it works as you'd expect (find the git-dir in the
> > current path, then the working tree via $GIT_WORK_TREE).
> 
> Hmm, does what is done by "git add HEAD" in such a situation match
> what you'd expect?
> 
> git init work
> cd work; date >HEAD; git commit -m initial
> git push ../bare master:master
>   date >>HEAD
> export GIT_WORK_TREE=$(pwd)
>   cd ..
>   git --bare init bare
>   cd bare
>   git add HEAD

I had to tweak your commands a little, but I assume the part you are
interested in is the end, when git-add finds HEAD in $GIT_WORK_TREE and
not the bare repository.

And yes, that is exactly what I'd expect, and why it is useful (if you
wanted to add arbitrary cruft from the bare repo, you'd set
$GIT_WORK_TREE to point there).

> I'd have to say that this invites unnecessary confusion, even though
> I agree that "go to the GIT_WORK_TREE and take pathspecs relative to
> that directory" is the only sensible thing for us to be doing.
> 
> But that is not an issue about "set only work-tree" (it is about
> "run from outside the work-tree").

Yeah, there are two things going on:

  1. Without $GIT_DIR but with $GIT_WORK_TREE, we find $GIT_DIR via the
 usual discovery path.

  2. When outside $GIT_WORK_TREE, any work-tree operations work as if
 they were started from $GIT_WORK_TREE.

And relying on (1) almost always relies on (2), unless your work-tree
happens to be inside the discovery path for your $GIT_DIR. So you could
do:

  git init repo
  mkdir repo/subdir
  echo content >file
  GIT_WORK_TREE=$(pwd) git add .

which adds "file" at the top-level. And we used only rule (1), not rule
(2). I don't know whether people actually do that or not (I guess it
could be useful for tricky subtree things).

-Peff
--
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 v1 6/7] correct blame for files commited with CRLF

[Junio C Hamano]

Torsten Bögershausen  writes:

> If we had made the CRLF -> LF conversion, yes. But we don't do that.
> crlf_to_git() returns without touching the line endings.

That sounds quite broken.  How would a user ever fix broken data in
the index then?  I know the commit that often appears in these
discussions claims to give us "safer CRLF" handling, but I have a
suspicion that perhaps we should rethink if that claim is really
true.  Isn't it giving us more problems than it is worth?

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

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

[John Keeping]

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

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

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

Re: [PATCH 2/5] t/t5520: explicitly unset rebase.autostash

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain  wrote:
> t/t5520: explicitly unset rebase.autostash

As with patch 1/5, this subject is written at too low a level, talking
about details of the patch rather than giving a high-level overview.
What the patch is really doing is ensuring consistent conditions
within the test even if some future change pollutes the global
configuration. Maybe:

t5520: ensure consistent test conditions

or:

t5520: make test expectations explicit

or something.

> Tests title suggest that tests are done with rebase.autostash unset,
> but doesn not take any action to make sure that it is indeed unset.

This is just paraphrasing my earlier review comment[1], however,
"suggest" is a weak argument for why this change is desirable. State
instead that this change ensures a consistent condition for tests in
which rebase.autostash should not be set and protects against some
future change polluting the global configuration.

> Make sure that rebase.autostash is unset by explicitly setting it.

The patch itself looks ok.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289860

> Signed-off-by: Mehul Jain 
> ---
>  t/t5520-pull.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 5be39df..9ee2218 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -279,6 +279,7 @@ test_expect_success 'pull --rebase --autostash & 
> rebase.autostash=false' '
>  '
>
>  test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
> +   test_unconfig rebase.autostash &&
> git reset --hard before-rebase &&
> echo dirty >new_file &&
> git add new_file &&
> @@ -307,6 +308,7 @@ test_expect_success 'pull --rebase --no-autostash & 
> rebase.autostash=false' '
>  '
>
>  test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
> +   test_unconfig rebase.autostash &&
> git reset --hard before-rebase &&
> echo dirty >new_file &&
> git add new_file &&
> --
> 2.7.1.340.g69eb491.dirty
--
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 4/5] t/t5520: modify tests to reduce common code

[Junio C Hamano]

Mehul Jain  writes:

> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.

Sound like sensible idea.

> +for i in true false
> + do
> + test_expect_success "pull --rebase --autostash & 
> rebase.autostash=$i" '
> + test_config rebase.autostash $i &&
> + test_rebase_autostash
> + '
> + done

The lines between do..done is over-indented (will locally fix--no
need to resend).

--
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] Fix http-backend reading till EOF, ignoring CONTENT_LENGTH, violating rfc3875 -- WAS: Problem with git-http-backend.exe as iis cgi

[Jeff King]

On Tue, Mar 29, 2016 at 10:38:23AM +, Florian Manschwetus wrote:

> > | A request-body is supplied with the request if the CONTENT_LENGTH is 
> > | not NULL.  The server MUST make at least that many bytes available 
> > | for the script to read.  The server MAY signal an end-of-file 
> > | condition after CONTENT_LENGTH bytes have been read or it MAY supply 
> > | extension data.  Therefore, the script MUST NOT attempt to read more 
> > | than CONTENT_LENGTH bytes, even if more data is available.  However, 
> > | it is not obliged to read any of the data.
> >
> > So yes, if Git currently reads until EOF, it's an error.
> > The correct way would be:
> >
> > 1) Check to see if the CONTENT_LENGTH variable is available in the
> >environment.  If no, read nothing.
> >
> > 2) Otherwise read as many bytes it specifies, and no more.
> >
> > 1. https://www.ietf.org/rfc/rfc3875

I don't think the second part of (1) will work very well if the client
sends a chunked transfer-encoding (which git will do if the input is large). In
such a case the server would either have to buffer the entire input to
find its length, or stream the data to the CGI without setting
$CONTENT_LENGTH. At least some servers choose the latter (including
Apache).

> diff --git a/http-backend.c b/http-backend.c
> index 8870a26..94976df 100644
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -277,16 +277,32 @@ static struct rpc_service *select_service(const char 
> *name)
>   */
>  static ssize_t read_request(int fd, unsigned char **out)
>  {
> - size_t len = 0, alloc = 8192;
> - unsigned char *buf = xmalloc(alloc);
> + unsigned char *buf = null;
> + size_t len = 0;
> + /* get request size */
> + size_t req_len = git_env_ulong("CONTENT_LENGTH",
> +0);
> +
> + /* check request size */
> + if (max_request_buffer < req_len) {
> + die("request was larger than our maximum size (%lu);"
> + " try setting GIT_HTTP_MAX_REQUEST_BUFFER",
> + max_request_buffer);
> + }
> +
> + if (req_len <= 0) {
> + *out = null;
> + return 0;
> + }

git-am complained that your patch did not apply, but after writing
something similar locally, I found that t5551.25 hangs indefinitely.
Which is not surprising. Most tests are doing very limited ref
negotiation, so the content that hits read_request() here is small, and
we send it in a single write with a content-length header. But t5551.25
uses a much bigger workload, which causes the client to use a chunked
transfer-encoding, and this code to refuse to read anything (and then
the protocol stalls, as we are waiting for the client to say something).

So I think you'd want to take a missing CONTENT_LENGTH as a hint to read
until EOF.

That also raises another issue: what happens in the paths that don't hit
read_request()? We may also process input via:

  - inflate_request(), if the client gzipped it; for well-formed input,
I think we'll stop reading when the gzip stream tells us there is no
more data, but a malformed one would have us reading until EOF,
regardless of what $CONTENT_LENGTH says.

  - for input which we expect to be large (like incoming packfiles for a
push), buffer_input will be unset, and we will pass the descriptor
directly to a sub-program like git-index-pack. Again, for
well-formed input it would read just the packfile, but it may
actually continue to EOF.

So I don't think your patch is covering all cases.

-Peff
--
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 00/21] replacement for dt/refs-backend-lmdb v7 patch 04/33

[David Turner]

On Sun, 2016-03-27 at 07:22 +0200, Michael Haggerty wrote:
> On 03/24/2016 07:47 AM, David Turner wrote:
> > [...]
> > I incorporated your changes into the lmdb backend.  To make merging
> > later more convenient, I rebased on top of pu -- I think this
> > mainly
> > depends on jk/check-repository-format, but I also included some
> > fixes
> > for a couple of tests that had been changed by other patches.
> 
> I think rebasing changes on top of pu is counterproductive. I believe
> that Junio had extra work rebasing your earlier series onto a merge
> of
> the minimum number of topics that it really depended on. There is no
> way
> that he could merge the branch in this form because it would imply
> merging all of pu.
> 
> See the zeroth section of SubmittingPatches [1] for the guidelines.

I'm a bit confused because 
[PATCH 18/21] get_default_remote(): remove unneeded flag variable

doesn't do anything on master -- it depends on some patch in pu.  And
we definitely want to pick up jk/check-repository-format (which doesn't
include whatever 18/21 depends on).

So what do you think our base should be?

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

Re: [PATCH 1/4] config --show-origin: report paths with forward slashes

[Junio C Hamano]

Johannes Sixt  writes:

> This part of your 45bf3297 (t1300: fix the new --show-origin tests on
> Windows)
>
> @@ -1205,6 +1205,9 @@ test_expect_success POSIXPERM,PERL 'preserves existing 
> per
>   "die q(badrename) if ((stat(q(.git/config)))[2] & 0) != 0600"
>  '
>  
> +! test_have_prereq MINGW ||
> +HOME="$(pwd)" # convert to Windows path
> +
>  test_expect_success 'set up --show-origin tests' '
> INCLUDE_DIR="$HOME/include" &&
> mkdir -p "$INCLUDE_DIR" &&
>
> is actually a much more concise version of my proposed patch,
> although the result still misuses $HOME where it does not have
> to. In fact, if I revert 5ca6b7bb (config --show-origin: report
> paths with forward slashes), the tests still pass. But since it
> does not make a difference save for a few microseconds more or
> less during startup, it is not worth the churn at this point.

Well, from the point of view of codebase cleanliness, if we can do
without 5ca6b7bb4, we would be much better off in the longer term,
so I would say it would be wonderful if we can safely revert 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 1/5] t/t5520: change rebase.autoStash to rebase.autostash

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain  wrote:
> t/t5520: change rebase.autoStash to rebase.autostash

This subject is written at too low a level, talking about details of
the patch rather than giving a high-level overview. A further
shortcoming is that there's no explanation of *why* this change is
desirable. Here's an attempt which addresses both problems.

t5520: use consistent capitalization in test titles

(Note that I dropped the leading "t/" since it's implied.)

The patch itself is fine.

> Signed-off-by: Mehul Jain 
> ---
>  t/t5520-pull.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 745e59e..5be39df 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -267,7 +267,7 @@ test_expect_success 'pull --rebase --autostash & 
> rebase.autostash=true' '
> test "$(cat file)" = "modified again"
>  '
>
> -test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
> +test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
> test_config rebase.autostash false &&
> git reset --hard before-rebase &&
> echo dirty >new_file &&
> @@ -278,7 +278,7 @@ test_expect_success 'pull --rebase --autostash & 
> rebase.autoStash=false' '
> test "$(cat file)" = "modified again"
>  '
>
> -test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
> +test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
> git reset --hard before-rebase &&
> echo dirty >new_file &&
> git add new_file &&
> --
> 2.7.1.340.g69eb491.dirty
--
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 v1 6/7] correct blame for files commited with CRLF

[Junio C Hamano]

Torsten Bögershausen  writes:

> # Here the lines are not going to be normalized at the next commit.
> # They stay CRLF.

Isn't that the real source of the problem?  Why don't we fix that
then?

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

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

[Junio C Hamano]

Jeff King  writes:

> On Tue, Mar 29, 2016 at 08:08:29AM -0700, Junio C Hamano wrote:
>
>> So it is a misconfiguration if you only set GIT_WORK_TREE without
>> setting GIT_DIR.
>
> Hmm. I have frequently done this when my cwd is a git repository (e.g.,
> a bare one), and it works as you'd expect (find the git-dir in the
> current path, then the working tree via $GIT_WORK_TREE).

Hmm, does what is done by "git add HEAD" in such a situation match
what you'd expect?

git init work
cd work; date >HEAD; git commit -m initial
git push ../bare master:master
date >>HEAD
export GIT_WORK_TREE=$(pwd)
cd ..
git --bare init bare
cd bare
git add HEAD

I'd have to say that this invites unnecessary confusion, even though
I agree that "go to the GIT_WORK_TREE and take pathspecs relative to
that directory" is the only sensible thing for us to be doing.

But that is not an issue about "set only work-tree" (it is about
"run from outside the work-tree").
--
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 v1 6/7] correct blame for files commited with CRLF

[Torsten Bögershausen]

On 29.03.16 19:21, Junio C Hamano wrote:
> tbo...@web.de writes:
> 
>> From: Torsten Bögershausen 
>>
>> git blame reports lines as not "Not Committed Yet" when they have
>> CRLF in the index, CRLF in the worktree and e.g. core.autocrlf is true.
>>
>> Since commit c48053 "new safer autocrlf handling", files that have CRLF
>> in the index are not normalized at commit when e.g. core.autocrl is set.
>>
>> Whenever a CLRF conversion is needed (or any filter us set), load the
>> index temporally, before calling convert_to_git()
> 
> Sorry, but I do not understand what problem you are trying to
> address here.
> 
> Under the same condition described in the first paragraph, what
> would "git diff" and "git diff HEAD" say?  They should show that you
> would be making a commit that corrects the line ending of the blob
> recorded in the history.
> 
Let's make an experiment, Git v2.8.0


$ printf "Line1\r\nLine2\r\n" >test_CRLF.txt
$ git add test_CRLF.txt 
$ git commit -m "add test_CRLF.txt"
 [detached HEAD 719c166] add test_CRLF.txt
 1 file changed, 2 insertions(+)
 create mode 100644 test_CRLF.txt

$ git ls-files --eol test_CRLF.txt 
i/crlf  w/crlf  attr/   test_CRLF.txt
# So far, so good.

git config core.autocrlf true

# Now lets patch Git to debug the safer CRLF handling
diff --git a/convert.c b/convert.c
index f524b8d..fcf7653 100644
--- a/convert.c
+++ b/convert.c
@@ -260,8 +260,11 @@ static int crlf_to_git(const char *path, const char *src, 
size_t len,
 * If the file in the index has any CR in it, do not 
convert.
 * This is the new safer autocrlf handling.
 */
-   if (has_cr_in_index(path))
+   if (has_cr_in_index(path)) {
+   fprintf(stderr, "%s/%s:%d 
has_cr_in_index(%s)\n",
+   __FILE__, __FUNCTION__, __LINE__, path);
return 0;
+   }

# Of course, run make
$ make
#
printf "Line3\r\n" >>test_CRLF.txt

# Lets see what diff says:
./git diff test_CRLF.txt | od -c
convert.c/crlf_to_git:265 has_cr_in_index(test_CRLF.txt)
convert.c/crlf_to_git:265 has_cr_in_index(test_CRLF.txt)
000d   i   f   f   -   -   g   i   t   a   /   t   e   s
020t   _   C   R   L   F   .   t   x   t   b   /   t   e   s
040t   _   C   R   L   F   .   t   x   t  \n   i   n   d   e   x
0604   a   a   5   5   1   d   .   .   d   0   f   a   f   1
100d   1   0   0   6   4   4  \n   -   -   -   a   /   t
120e   s   t   _   C   R   L   F   .   t   x   t  \n   +   +   +
140b   /   t   e   s   t   _   C   R   L   F   .   t   x   t
160   \n   @   @   -   1   ,   2   +   1   ,   3   @   @
200   \n   L   i   n   e   1  \r  \n   L   i   n   e   2  \r
220   \n   +   l   i   n   e   3  \r  \n
231
# Here the lines are not going to be normalized at the next commit.
# They stay CRLF.
# But git blame doesn't know that, because has_cr_in_index doesn't work
# without an index.

$ ./git blame test_CRLF.txt 
 (Not Committed Yet 2016-03-29 21:44:48 +0200 1) Line1
 (Not Committed Yet 2016-03-29 21:44:48 +0200 2) Line2
 (Not Committed Yet 2016-03-29 21:44:48 +0200 3) line3



$ git commit -m "Add line3" test_CRLF.txt

> The "Not Committed Yet" output from "git blame" is the same thing.
> It is telling you that the commit you would be making by adding
> that path from the working tree in its current state will become
> the one that is responsible for the final state of the line.
> 
> So it is absolutely the right thing that these lines are shown as
> "Not Commited Yet".  You will be making the line-ending correction
> for the entire blob, and you should be made aware of it.
If we had made the CRLF -> LF conversion, yes. But we don't do that.
crlf_to_git() returns without touching the line endings.



--
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 4/7] submodule update --init: correct path handling in recursive submodules

[Stefan Beller]

On Tue, Mar 29, 2016 at 12:46 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This fixes the test introduced by the parent commit.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>
> The first hunk in this patch touches lines that goes away with
> d5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on
> your sb/submodule-init topic and the whole block is replaced
> by a call to "submodule--helper init".
>
> I'll drop the first hunk when merging this series to 'pu' for now;
> hopefully you did not inherit the bug when rewriting the part into
> "submodule--helper init".

After examining this patch more closely for a better commit message, the second
hunk also goes away, i.e. only the test will remain.

Maybe I can roll this series on top of origin/sb/submodule-init, to
avoid confusion.

>
> Thanks.
>
>>  git-submodule.sh| 5 +++--
>>  t/t7406-submodule-update.sh | 2 +-
>>  2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 2838069..a7c8599 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -474,7 +474,7 @@ cmd_init()
>>   die_if_unmatched "$mode"
>>   name=$(git submodule--helper name "$sm_path") || exit
>>
>> - displaypath=$(relative_path "$sm_path")
>> + displaypath=$(relative_path "$prefix$sm_path")
>>
>>   # Copy url setting when it is not set yet
>>   if test -z "$(git config "submodule.$name.url")"
>> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
>>   if test -n "$recursive"
>>   then
>>   (
>> - prefix="$prefix$sm_path/"
>> + prefix="$(relative_path $prefix$sm_path)/"
>>   clear_local_git_env
>> + wt_prefix=
>>   cd "$sm_path" &&
>>   eval cmd_update
>>   )
>> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
>> index c1b9ffa..3bd1552 100755
>> --- a/t/t7406-submodule-update.sh
>> +++ b/t/t7406-submodule-update.sh
>> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out 
>> '${rebasingsha1}'
>>  Submodule path '../super/submodule': checked out '${submodulesha1}'
>>  EOF
>>
>> -test_expect_failure 'submodule update --init --recursive from subdirectory' 
>> '
>> +test_expect_success 'submodule update --init --recursive from subdirectory' 
>> '
>>   git -C recursivesuper/super reset --hard HEAD^ &&
>>   (cd recursivesuper &&
>>mkdir tmp &&
--
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 4/7] submodule update --init: correct path handling in recursive submodules

[Junio C Hamano]

Stefan Beller  writes:

> This fixes the test introduced by the parent commit.
>
> Signed-off-by: Stefan Beller 
> ---

The first hunk in this patch touches lines that goes away with
d5bc3cd2 (submodule: port init from shell to C, 2016-03-14) on
your sb/submodule-init topic and the whole block is replaced
by a call to "submodule--helper init".

I'll drop the first hunk when merging this series to 'pu' for now;
hopefully you did not inherit the bug when rewriting the part into
"submodule--helper init".

Thanks.

>  git-submodule.sh| 5 +++--
>  t/t7406-submodule-update.sh | 2 +-
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 2838069..a7c8599 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -474,7 +474,7 @@ cmd_init()
>   die_if_unmatched "$mode"
>   name=$(git submodule--helper name "$sm_path") || exit
>  
> - displaypath=$(relative_path "$sm_path")
> + displaypath=$(relative_path "$prefix$sm_path")
>  
>   # Copy url setting when it is not set yet
>   if test -z "$(git config "submodule.$name.url")"
> @@ -826,8 +826,9 @@ Maybe you want to use 'update --init'?")"
>   if test -n "$recursive"
>   then
>   (
> - prefix="$prefix$sm_path/"
> + prefix="$(relative_path $prefix$sm_path)/"
>   clear_local_git_env
> + wt_prefix=
>   cd "$sm_path" &&
>   eval cmd_update
>   )
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index c1b9ffa..3bd1552 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -118,7 +118,7 @@ Submodule path '../super/rebasing': checked out 
> '${rebasingsha1}'
>  Submodule path '../super/submodule': checked out '${submodulesha1}'
>  EOF
>  
> -test_expect_failure 'submodule update --init --recursive from subdirectory' '
> +test_expect_success 'submodule update --init --recursive from subdirectory' '
>   git -C recursivesuper/super reset --hard HEAD^ &&
>   (cd recursivesuper &&
>mkdir tmp &&
--
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 v1 5/7] CRLF: unify the "auto" handling

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:25 AM,   wrote:
> Make .gitattributes "* text=auto eol=crlf" to do the same as
> setting core.autocrlf=true and "* text=auto eol=crlf" the same
> as core.autocrlf=input
>
> Signed-off-by: Torsten Bögershausen 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -389,14 +389,15 @@ file with mixed line endings would be reported by the 
> `core.safecrlf`
>  core.autocrlf::
> -   Setting this variable to "true" is almost the same as setting
> -   the `text` attribute to "auto" on all files except that text
> -   files are not guaranteed to be normalized: files that contain
> +   Setting this variable to "true" is the same as setting
> +   the attributes to "auto eol=crlf" on all files.
> +   Files are not guaranteed to be normalized: files that contain
> `CRLF` in the repository will not be touched.  Use this
> setting if you want to have `CRLF` line endings in your
> -   working directory even though the repository does not have
> -   normalized line endings.  This variable can be set to 'input',
> +   working directory and he repository has normalized line endings.

s/he/the/

> +   This variable can be set to 'input',
> in which case no output conversion is performed.
> +   Note: older versions of Git behave different.

Would it make sense to spell out what "different" means since new
readers won't know what the old behavior was?

> diff --git a/convert.c b/convert.c
> @@ -370,12 +369,10 @@ static int crlf_to_worktree(const char *path, const 
> char *src, size_t len,
> if (crlf_action == CRLF_AUTO || crlf_action == CRLF_AUTO_INPUT || 
> crlf_action == CRLF_AUTO_CRLF) {
> -   if (crlf_action == CRLF_AUTO_INPUT || crlf_action == 
> CRLF_AUTO_CRLF) {
> -   /* If we have any CR or CRLF line endings, we do not 
> touch it */
> -   /* This is the new safer autocrlf-handling */
> -   if (stats.lonecr || stats.crlf )
> -   return 0;
> -   }
> +   /* If we have any CR or CRLF line endings, we do not touch it 
> */
> +   /* This is the new safer autocrlf-handling */

The comment style violation could have been fixed while outdenting,
but perhaps it's not worth the churn.

> +   if (stats.lonecr || stats.crlf )
> +   return 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: `git rev-parse --is-inside-work-tree` and $GIT_WORK_TREE

[Jeff King]

On Tue, Mar 29, 2016 at 08:08:29AM -0700, Junio C Hamano wrote:

> So it is a misconfiguration if you only set GIT_WORK_TREE without
> setting GIT_DIR.

Hmm. I have frequently done this when my cwd is a git repository (e.g.,
a bare one), and it works as you'd expect (find the git-dir in the
current path, then the working tree via $GIT_WORK_TREE).

I always assumed that was the intended behavior, as it is the only one
that makes sense. I suspect I am not alone in having relied on this.

> Also, if you set both and run Git from outside $GIT_WORK_TREE, even
> though Git may try to do its best to give you a reasonable behaviour
> [*1*], it is working by accident not by design (see the statement
> you are making by setting GIT_WORK_TREE in the third bullet above).
> 
> IOW, running from outside GIT_WORK_TREE is a misconfiguration.
> 
> [Footnote]
> 
> *1* Think what should happen when you are outside GIT_WORK_TREE and
> say this:
> 
>   $ git grep foo
> 
> As you are not even inside the working tree, the command would
> not know in which subdirectory you want to find the string foo;
> the "reasonable behaviour" is to work on the whole working tree
> in this case.

Likewise, I always assumed this "reasonable behavior" was intended. When
we setup_git_directory(), we end up in the root of the working tree as
usual. The "prefix" must be empty, as we were not in the work tree at
all, and we do a whole-tree operation.

Those behaviors may not have been fully designed, but as they do the
only reasonable thing (besides dying with an error), and people may have
baked that assumption into their scripts, I think we should avoid
changing them unless there is a compelling reason.

-Peff
--
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 v1 1/7] Make it possible to get sha1 for a path from the index

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:25 AM,   wrote:
> Factor out the retrival of the sha1 for a given path in

s/retrival/retrieval/

> read_blob_data_from_index() into the function get_sha1_from_index().
>
> This will be used in the next commit, when convert.c can do the analyze
> for "text=auto" without slurping the whole blob into memory at once.
>
> Add a wrapper definition get_sha1_from_cache().
>
> Signed-off-by: Torsten Bögershausen 
--
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