[PATCH] t6044: replace seq by test_seq

[Johannes Sixt]

seq is not available everywhere.

Signed-off-by: Johannes Sixt 
---
 t/t6044-merge-unrelated-index-changes.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t6044-merge-unrelated-index-changes.sh 
b/t/t6044-merge-unrelated-index-changes.sh
index 20a3ffe..0102348 100755
--- a/t/t6044-merge-unrelated-index-changes.sh
+++ b/t/t6044-merge-unrelated-index-changes.sh
@@ -20,7 +20,7 @@ test_description="merges with unrelated index changes"
 #   Commit E: renames a->subdir/a, adds subdir/e
 
 test_expect_success 'setup trivial merges' '
-   seq 1 10 >a &&
+   test_seq 1 10 >a &&
git add a &&
test_tick && git commit -m A &&
 
@@ -42,7 +42,7 @@ test_expect_success 'setup trivial merges' '
test_tick && git commit -m C &&
 
git checkout D &&
-   seq 2 10 >a &&
+   test_seq 2 10 >a &&
echo d >d &&
git add a d &&
test_tick && git commit -m D &&
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "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] fast-import: do not truncate exported marks file

[Junio C Hamano]

Felipe Contreras  writes:

> On Tue, May 17, 2016 at 10:59 PM, Junio C Hamano  wrote:
>> On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras
>>  wrote:
>>> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano  wrote:
>
  - Even if we did not read from any existing marks file, if we are
given export_marks_file that names an existing file, wouldn't we
want to avoid corrupting it with a dump from this aborted run?
>>>
>>> If we don't run from an existing marks file, this patch has no effect.
>>
>> Yes, that is exactly what I was wondering if we may want to improve
>> while at it.
>
> This doesn't make much sense. Corrupted from where? This patch is
> tackling the issue where the imported marks file is "corrupted".

s/corrupted/overwritten by garbage/ is what I really meant, but in
any case, I do not think there is any valid use case that relied on
the current behaviour that will be broken by this update.  I do not
think a script that used separate import and export marks files (and
used mv to rename the exported one to the import used by the next
round, only after seeing fast-import successfully exits) would be
harmed, as a non-zero status is still a non-exit status and it
wouldn't have moved a corrupt export file to the next import, for
example.  Other people and real users of fast-import might find
cases I couldn't think of that this change negatively affects, but
that's expected for any change and that's why we cook any changes in
'pu' and then 'next'.

So let's queue this as a strict improvement.

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 v4 2/2] convert: ce_compare_data() checks for a sha1 of a path

[Torsten Bögershausen]

On 05/17/2016 08:58 PM, Junio C Hamano wrote:

tbo...@web.de writes:


  #define HASH_WRITE_OBJECT 1
  #define HASH_FORMAT_CHECK 2
+#define HASH_CE_HAS_SHA1  4


How does one pronounce the words in this constant?  Does it make a
listener understand what this constant means?

How about
HASH_USE_SHA1_FROM_CE
or
HASH_CE_HAS_VALID_SHA1





/*
  * We need a comment around here to say what these two
  * parameters mean.  I am guessing that (1) if sha1 is not NULL,
  * path is ignored and the function inspects if it has CR; (2)
  * otherwise it checks the index entry at path and inspects if
  * it has CR.
  */


-static int has_cr_in_index(const char *path)
+static int has_cr_in_index(const char *path, const unsigned char *sha1)
  {


This makes me seriously wonder if it is a good idea to modify this
function like this.  (1) means this function is not about IN INDEX
at all!

Perhaps add a "static int blob_has_cr(const unsigned char *sha1)"
and call it from the real caller you added that wants to call this
butchered two-param version that has sha1 is a better solution?


-static int crlf_to_git(const char *path, const char *src, size_t len,
+static int crlf_to_git(const char *path, const unsigned char *sha1,
+  const char *src, size_t len,
   struct strbuf *buf,
   enum crlf_action crlf_action, enum safe_crlf checksafe)
  {
@@ -260,7 +267,7 @@ 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, sha1))




I think this change is too ugly.  The new "sha1" parameter is
telling us that "in order to see if the indexed version has CR, do
not look at the index, but look at the contents of this blob".

Thanks for some fresh eyes, I guess that
blob_has_cr(sha1)
would make most sense.

--
To unsubscribe from this list: send the line "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] fast-import: do not truncate exported marks file

[Felipe Contreras]

On Tue, May 17, 2016 at 10:59 PM, Junio C Hamano  wrote:
> On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras
>  wrote:
>> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano  wrote:

>>>  - Even if we did not read from any existing marks file, if we are
>>>given export_marks_file that names an existing file, wouldn't we
>>>want to avoid corrupting it with a dump from this aborted run?
>>
>> If we don't run from an existing marks file, this patch has no effect.
>
> Yes, that is exactly what I was wondering if we may want to improve
> while at it.

This doesn't make much sense. Corrupted from where? This patch is
tackling the issue where the imported marks file is "corrupted".

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line "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] fast-import: do not truncate exported marks file

[Junio C Hamano]

On Tue, May 17, 2016 at 8:31 PM, Felipe Contreras
 wrote:
> On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano  wrote:
>> Felipe Contreras  writes:
>>
>>  - Even if we are reading from somewhere, export_marks_file can
>>point at a completely new file that is different from
>>import_marks file, in which case, we are not really losing any
>>information by freshly creating a new marks file, no?
>
> Right, we are not losing any information, but we are not gaining much
> either: it's a truncated version of the import marks.

Ah, if we finished reading from the marks file and then die elsewhere,
the new conditional that says "don't clobber if we haven't finished reading"
would not have any effect, right. We'll see the imported ones plus some
of the new ones.

And if we didn't read anything but have export defined, then we can overwrite
the existing marks file, but that is nothing new.

>>  - Even if we did not read from any existing marks file, if we are
>>given export_marks_file that names an existing file, wouldn't we
>>want to avoid corrupting it with a dump from this aborted run?
>
> If we don't run from an existing marks file, this patch has no effect.

Yes, that is exactly what I was wondering if we may want to improve
while at it.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

On Tue, May 17, 2016 at 7:06 PM, SZEDER Gábor  wrote:
> Quoting Junio C Hamano :
>> Jeff King  writes:
>>> On Tue, May 17, 2016 at 04:48:33PM -0400, Eric Sunshine wrote:
 On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano 
 wrote:
> Eric Sunshine  writes:
>> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&
>
> This is kosher POSIX, but I vaguely recall some shells had trouble
> with the SP between -C and "$dir" in the past.  Let's see if anybody
> screams; hopefully I am misremembering or buggy shells died out.

 I also am bothered by a vague recollection of some issue (possibly
 involving the internal space and lack of quotes around the entire
 ${...}), but couldn't remember nor find a reference to the specific
 details. Perhaps someone reading the patch has a better memory than I.
>>>
>>> Probably:
>>>  http://thread.gmane.org/gmane.comp.version-control.git/265094
>
> And ea10b60c910e (Work around ash "alternate value" expansion bug,
> 2009-04-18) as well.
>
>http://thread.gmane.org/gmane.comp.version-control.git/116816

Thanks for the additional link. I have v3 ready to roll and will send
it out within the next day if no more actionable review comments
arrive.
--
To unsubscribe from this list: send the line "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] fast-import: do not truncate exported marks file

[Felipe Contreras]

On Tue, May 17, 2016 at 5:22 PM, Junio C Hamano  wrote:
> Felipe Contreras  writes:
>
>> Certain lines of the marks file might be corrupted (or the objects
>> missing due to a garbage collection), but that's no reason to truncate
>> the file and essentially destroy the rest of it.
>
> Hmm, so the issue is:
>
>  - we use die_nicely() that calls dump_marks() after writing a crash
>report as our die_routine().
>
>  - when dump_marks() is called, and export_marks_file names an
>existing file, it tries to write marks in it.  If we let it go
>through, we would end up writing a new marks file based on an
>incomplete set of marks we have only half-read in the earlier
>step, which is bad, as the resulting one is incomplete, and the
>original one that this replaced may have been a good one.
>
> Is that what this change addresses?

Yes. As I said; the marks file gets truncated.

> I am just wondering if a solution to preserve both files is more
> desirable.
>
> This change looks a bit over-eager to discard the dump die_nicely()
> is trying to create in one scenario, and a bit less careful at the
> same time in another scenario.
>
>  - Even if we are reading from somewhere, export_marks_file can
>point at a completely new file that is different from
>import_marks file, in which case, we are not really losing any
>information by freshly creating a new marks file, no?

Right, we are not losing any information, but we are not gaining much
either: it's a truncated version of the import marks.

>  - Even if we did not read from any existing marks file, if we are
>given export_marks_file that names an existing file, wouldn't we
>want to avoid corrupting it with a dump from this aborted run?

If we don't run from an existing marks file, this patch has no effect.

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

Ignore updates to files which are ignored, even if they're indexed.

[Josh McCullough]

TL;DR: Git should ignore files if they are in `.gitignore`, regardless
of whether or not the file has changed (or been removed).

Use Case

An initial set of "local" config files are committed to a repo. Each
developer will pull down this set of files, but changes will be
auto-ignored unless the file is force-added after a change is made.

Current Workarounds

1) Use `git update-index --assume-unchanged local-config.xyz` to
ignore local changes to the file.
2) Add templates instead (local-config.template.xyz) and tell each
developer to copy these templates and remove the ".template" portion
of the file name. The file name without the ".template" portion is
added to `.gitignore`.

The current workarounds rely on the developer to know/remember what to
do. It would be nice if we could do this instead:

1) Create the initial `config-local.xyz`.
2) Add the new file to the index (`git add`).
3) Add the same path to `.gitignore`, and add the changes to the index.
4) Commit the staged changes.
5) Edit the `config-local.xyz` file.
6) Run `git status` -- git will not suggest that the file was changed
since it is in the `.gitignore`. If 7) The user specifically made
changes they wish to commit, the can force-add the ignored file.

Doing a search for [git commit file only once then ignore changes]
will show that other people wish for the same feature.


Thanks!


- - -

Side note: It took me 4 tries to get this email to go through, since
non-plain-text messages are strictly forbidden by this mailing list. I
use Inbox, which doesn't have an option to ensure a message is sent as
plain text. I had to switch back to GMail to get it to go through.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Bug] git-log prints wrong unixtime with --date=format:%s

[Jeff King]

On Tue, May 17, 2016 at 08:40:08PM -0400, Jeff King wrote:

> So we need some way to tell strftime "...and by the way, this is the
> timezone for the value we are currently feeding you." There isn't a slot
> in "struct tm" for that, but I think maybe you can hack around it by
> setting the global "timezone" variable (and then restoring it
> afterwards).

I tried a few obvious things, but couldn't make anything work. Setting
"timezone" manually seems to do nothing. It's supposed to be set by
putting the right thing in $TZ and then calling tzset(). So I tried
munging $TZ to something like "+0200". It did have _some_ effect, but I
couldn't get it to behave correctly in all situations. And using "%z"
and "%Z" always just gives GMT.

So I dunno. Perhaps there is some way to coax sane behavior out of
strftime, but I could not figure it out. Suggestions welcome.

-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: [Bug] git-log prints wrong unixtime with --date=format:%s

[Jeff King]

On Tue, May 17, 2016 at 07:25:31PM +0200, Michael Heerdegen wrote:

> Michael Heerdegen  writes:
> 
> > the command
> >
> >git log --pretty=format:%ad --date=format:%s
> >
> > displays wrong unixtime values; apparently how much the printed value
> > differs from the expected value depends on the system's time zone and
> > whether daylight savings time is enabled or not.
> 
> Two more comments:
> 
>   - --date=raw seems to work correctly (though the doc tells it would
> also use "%s").

I think the doc is misleading there. None of the date formats except the
recently added "--date=format" use strftime at all.

The problem is almost certainly some mismatch between git's idea of the
timezone and what we are feeding to strftime. I'm not sure if it will be
solvable, though. strftime() takes a "struct tm", which has no timezone
field. We feed it a "struct tm" either in the author's timezone or in
the user's timezone (if "format-local" was used). We could feed it in
UTC, but I'm not sure that solves the problem, and it probably makes new
ones.

In your examples, you showed that setting $TZ changes the epoch time
strftime gives us. Which probably means that it is guessing the fed
"struct tm" is in the local timezone, which would still be wrong.

And even if that did work, feeding it UTC means that all of the other
non-%s values would be in UTC, not in the author's or user's timezone.

So we need some way to tell strftime "...and by the way, this is the
timezone for the value we are currently feeding you." There isn't a slot
in "struct tm" for that, but I think maybe you can hack around it by
setting the global "timezone" variable (and then restoring it
afterwards).

>   - When specifying versions like @{N hours}, the result seems to differ
> (by one hour?) from what I expect, too.

This is probably a totally separate issue, as it would not be using
strftime (or IIRC, any of the standard time functions at all). Do you
have a detailed example that shows the problem?

-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 v2 3/5] t1500: avoid changing working directory outside of tests

[SZEDER Gábor]

Quoting Junio C Hamano :


Jeff King  writes:


On Tue, May 17, 2016 at 04:48:33PM -0400, Eric Sunshine wrote:


On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano  wrote:

Eric Sunshine  writes:

+ git ${dir:+-C "$dir"} rev-parse --$o >actual &&


This is kosher POSIX, but I vaguely recall some shells had trouble
with the SP between -C and "$dir" in the past.  Let's see if anybody
screams; hopefully I am misremembering or buggy shells died out.


I also am bothered by a vague recollection of some issue (possibly
involving the internal space and lack of quotes around the entire
${...}), but couldn't remember nor find a reference to the specific
details. Perhaps someone reading the patch has a better memory than I.


Probably:

 http://thread.gmane.org/gmane.comp.version-control.git/265094


And ea10b60c910e (Work around ash "alternate value" expansion bug,
2009-04-18) as well.

   http://thread.gmane.org/gmane.comp.version-control.git/116816


Yikes, you're right.  Does anybody know if FreeBSD shell is still
buggy?


git-submodule.sh contains a few offending constructs:

git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
"$@" || echo "#unmatched"
} | {

They were added recently in 48308681b072 (git submodule update: have
a dedicated helper for cloning, 2016-02-29), which is not in any
release yet, perhaps that's why noone complained yet.

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

Re: [PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Junio C Hamano]

Jeff King  writes:

> On Tue, May 17, 2016 at 04:48:33PM -0400, Eric Sunshine wrote:
>
>> On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano  wrote:
>> > Eric Sunshine  writes:
>> >> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&
>> >
>> > This is kosher POSIX, but I vaguely recall some shells had trouble
>> > with the SP between -C and "$dir" in the past.  Let's see if anybody
>> > screams; hopefully I am misremembering or buggy shells died out.
>> 
>> I also am bothered by a vague recollection of some issue (possibly
>> involving the internal space and lack of quotes around the entire
>> ${...}), but couldn't remember nor find a reference to the specific
>> details. Perhaps someone reading the patch has a better memory than I.
>
> Probably:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/265094

Yikes, you're right.  Does anybody know if FreeBSD shell is still
buggy?


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

Re: [PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

On Tue, May 17, 2016 at 5:52 PM, Jeff King  wrote:
> On Tue, May 17, 2016 at 04:48:33PM -0400, Eric Sunshine wrote:
>> On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano  wrote:
>> > Eric Sunshine  writes:
>> >> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&
>> >
>> > This is kosher POSIX, but I vaguely recall some shells had trouble
>> > with the SP between -C and "$dir" in the past.  Let's see if anybody
>> > screams; hopefully I am misremembering or buggy shells died out.
>>
>> I also am bothered by a vague recollection of some issue (possibly
>> involving the internal space and lack of quotes around the entire
>> ${...}), but couldn't remember nor find a reference to the specific
>> details. Perhaps someone reading the patch has a better memory than I.
>
>   http://thread.gmane.org/gmane.comp.version-control.git/265094

Thanks for the link. I just tested with FreeBSD 8, and the shell
indeed exhibits that broken behavior. The workaround in Kyle's patch
isn't the prettiest (and is a bit verbose), but it gets the job done.

I can send v3 using that workaround unless Junio wants to patch it locally(?).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

What's cooking in git.git (May 2016, #06; Tue, 17)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

The 'master' branch now has 390 non-merge commits in this cycle.  On
the 'maint' front, 2.8.2 is out and fixes that have been in 'master'
accumulates on it for 2.8.3, which probably should be tagged sometime
late this week.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* ab/hooks (2016-05-04) 4 commits
  (merged to 'next' on 2016-05-09 at 23cf808)
 + hooks: allow customizing where the hook directory is
 + githooks.txt: minor improvements to the grammar & phrasing
 + githooks.txt: amend dangerous advice about 'update' hook ACL
 + githooks.txt: improve the intro section

 A new configuration variable core.hooksPath allows customizing
 where the hook directory is.


* ak/t4151-ls-files-could-be-empty (2016-05-09) 1 commit
  (merged to 'next' on 2016-05-10 at 36ae38c)
 + t4151: make sure argument to 'test -z' is given

 Test fix.


* bn/config-doc-tt-varnames (2016-05-05) 1 commit
  (merged to 'next' on 2016-05-10 at aa7b834)
 + config: consistently format $variables in monospaced font
 (this branch uses jc/config-pathname-type.)

 Doc formatting fixes.


* bn/http-cookiefile-config (2016-05-04) 2 commits
  (merged to 'next' on 2016-05-09 at d519b13)
 + http: expand http.cookieFile as a path
 + Documentation: config: improve word ordering for http.cookieFile

 "http.cookieFile" configuration variable clearly wants a pathname,
 but we forgot to treat it as such by e.g. applying tilde expansion.


* es/test-gpg-tags (2016-05-09) 1 commit
  (merged to 'next' on 2016-05-10 at 9fcb98b)
 + t6302: simplify non-gpg cases

 Test fix.


* jc/config-pathname-type (2016-05-04) 1 commit
  (merged to 'next' on 2016-05-09 at 0876e55)
 + config: describe 'pathname' value type
 (this branch is used by bn/config-doc-tt-varnames.)

 Consolidate description of tilde-expansion that is done to
 configuration variables that take pathname to a single place.


* jc/fsck-nul-in-commit (2016-05-10) 2 commits
  (merged to 'next' on 2016-05-10 at 3bc3ca3)
 + fsck: detect and warn a commit with embedded NUL
 + fsck_commit_buffer(): do not special case the last validation

 "git fsck" learned to catch NUL byte in a commit object as
 potential error and warn.


* jc/linkgit-fix (2016-05-09) 1 commit
  (merged to 'next' on 2016-05-10 at 0e5ba60)
 + Documentation: fix linkgit references

 Many 'linkgit:' references were broken,
 which are all fixed with this.


* jc/ll-merge-internal (2016-05-09) 3 commits
  (merged to 'next' on 2016-05-10 at a6bf1d0)
 + t6036: remove pointless test that expects failure
 + ll-merge: use a longer conflict marker for internal merge
 + ll-merge: fix typo in comment

 "git rerere" can get confused by conflict markers deliberately left
 by the inner merge step, because they are indistinguishable from
 the real conflict markers left by the outermost merge which are
 what the end user and "rerere" need to look at.  This was fixed by
 making the conflict markers left by the inner merges a bit longer.


* jc/test-seq (2016-05-09) 2 commits
  (merged to 'next' on 2016-05-10 at 1512890)
 + test-lib-functions.sh: rewrite test_seq without Perl
 + test-lib-functions.sh: remove misleading comment on test_seq

 Test fix.


* jk/rebase-interative-eval-fix (2016-05-10) 1 commit
  (merged to 'next' on 2016-05-11 at 4fdf387)
 + rebase--interactive: avoid empty list in shell for-loop

 Portability enhancement for "rebase -i" to help platforms whose
 shell does not like "for i in " (which is not POSIX-kosher).


* jk/submodule-c-credential (2016-05-06) 6 commits
  (merged to 'next' on 2016-05-10 at 4abe871)
 + submodule: stop sanitizing config options
 + submodule: use prepare_submodule_repo_env consistently
 + submodule--helper: move config-sanitizing to submodule.c
 + submodule: export sanitized GIT_CONFIG_PARAMETERS
 + t5550: break submodule config test into multiple sub-tests
 + t5550: fix typo in $HTTPD_URL
 (this branch is used by js/http-custom-headers.)

 An earlier addition of "sanitize_submodule_env" with 14111fc4 (git:
 submodule honor -c credential.* from command line, 2016-02-29)
 turned out to be a convoluted no-op; implement what it wanted to do
 correctly, and stop filtering settings given via "git -c var=val".


* jk/test-send-sh-x-trace-elsewhere (2016-05-11) 1 commit
  (merged to 'next' on 2016-05-11 at 273a137)
 + test-lib: set BASH_XTRACEFD automatically

 Running tests with '-x' option to trace the individual command
 executions is a useful way to debug test scripts, but some tests
 that capture the standard error stream and check what the command
 said can 

Re: [PATCH 04/14] connect: rewrite feature parsing to work on string_list

[David Turner]

On Wed, 2016-05-04 at 13:13 -0700, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
> > Later on when we introduce the version 2 transport protocol, the
> > capabilities will not be transported in one lone string but each
> 
> s/lone/long/, I think.
> 
> > capability will be carried in its own pkt line.
> > 
> > To reuse existing infrastructure we would either need to join the
> > capabilities into a single string again later or refactor the
> > current
> > capability parsing to be using a data structure which fits both
> > versions of the transport protocol. We chose to implement the
> > later.
> > 
> > Signed-off-by: Stefan Beller 
> > ---
> >  builtin/receive-pack.c | 15 ++---
> >  connect.c  | 82 +++---
> > 
> >  connect.h  |  2 +-
> >  upload-pack.c  | 13 ++--
> >  4 files changed, 58 insertions(+), 54 deletions(-)
> 
> I am not sure if the churn is make a right tradeoff here.
> 
> A loop to concatenate each segment into a strbuf before passing it
> to parse_feature_request would be at most 5 lines long, no?

I started looking at this again briefly today, and I actually think
that string manipulation of this sort is a mistake.  String
manipulation is error-prone, and having an interface defined in terms
of strings makes it hard for a reader to see what sorts of input are
valid and invalid.   (It's also often somewhat less efficient).  So I
think it is a good idea to improve the interface while we're already
changing code in this area.
--
To unsubscribe from this list: send the line "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] fast-import: do not truncate exported marks file

[Junio C Hamano]

Felipe Contreras  writes:

> Certain lines of the marks file might be corrupted (or the objects
> missing due to a garbage collection), but that's no reason to truncate
> the file and essentially destroy the rest of it.

Hmm, so the issue is:

 - we use die_nicely() that calls dump_marks() after writing a crash
   report as our die_routine().

 - when dump_marks() is called, and export_marks_file names an
   existing file, it tries to write marks in it.  If we let it go
   through, we would end up writing a new marks file based on an
   incomplete set of marks we have only half-read in the earlier
   step, which is bad, as the resulting one is incomplete, and the
   original one that this replaced may have been a good one.

Is that what this change addresses?

I am just wondering if a solution to preserve both files is more
desirable.

This change looks a bit over-eager to discard the dump die_nicely()
is trying to create in one scenario, and a bit less careful at the
same time in another scenario.

 - Even if we are reading from somewhere, export_marks_file can
   point at a completely new file that is different from
   import_marks file, in which case, we are not really losing any
   information by freshly creating a new marks file, no?

 - Even if we did not read from any existing marks file, if we are
   given export_marks_file that names an existing file, wouldn't we
   want to avoid corrupting it with a dump from this aborted run?

In other words, I understand the intent of "import-marks-file-done",
but I am not sure if that is so special a case.

If the patch were to tell dump_marks() if the caller is die_nicely()
or not, and stop it from overwriting an existing file, whether we
read from any import-marks file, I would agree with the change a lot
more strongly.  An obvious extension of that line of thought would
be to export to an alternative marks file, perhaps to
strbuf_addf("%s.crash", exports_marks_file), if exports_marks_file
already exists if we are called while dying.

--
To unsubscribe from this list: send the line "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] A part of an edge from an octopus merge gets colored, even with --color=never

[Jeff King]

On Tue, May 17, 2016 at 10:02:22PM +0200, Johannes Sixt wrote:

> > Interesting. It replicates out of the box for me.
> 
> "Out of the box" are the magic words. I usually compile with -O0, which
> doesn't trigger the valgrind report.

Heh, I meant that Noam's test worked out of the box. I also build with
-O0. I was able to replicate with different optimization levels.

I think the interesting thing here is actually the libc (and therefore
possibly your valgrind version). I tried compiling with ASAN and get a
color value of "48830". But no ASAN warning!

I think what is happening is that we over-allocate the new_columns array
based on a power of 2, but only initialize up to num_new_columns. So the
off-by-one accesses heap memory that is allocated but which we have
never written to.

> When I compile with a 3.x based gcc on Windows, I see these warnings:
> 
> CC color.o
> color.c: In function 'color_parse_mem':
> color.c:203: warning: 'c.value' may be used uninitialized in this function
> color.c:203: warning: 'c.blue' may be used uninitialized in this function
> color.c:203: warning: 'c.green' may be used uninitialized in this function
> color.c:203: warning: 'c.red' may be used uninitialized in this function
> 
> (which triggered my curiosity in this bug report). But they seem to be
> unrelated and are most likely false positives.

Yeah, I think that's unrelated. I'd be highly distrustful of
-Wuninitialized in gcc 3.x. We had to mark quite a few false positives
back then, that were later corrected in the 4.x series.

-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 v2 3/5] t1500: avoid changing working directory outside of tests

[Jeff King]

On Tue, May 17, 2016 at 04:48:33PM -0400, Eric Sunshine wrote:

> On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano  wrote:
> > Eric Sunshine  writes:
> >> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&
> >
> > This is kosher POSIX, but I vaguely recall some shells had trouble
> > with the SP between -C and "$dir" in the past.  Let's see if anybody
> > screams; hopefully I am misremembering or buggy shells died out.
> 
> I also am bothered by a vague recollection of some issue (possibly
> involving the internal space and lack of quotes around the entire
> ${...}), but couldn't remember nor find a reference to the specific
> details. Perhaps someone reading the patch has a better memory than I.

Probably:

  http://thread.gmane.org/gmane.comp.version-control.git/265094

-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

[PATCH] fast-import: do not truncate exported marks file

[Felipe Contreras]

Certain lines of the marks file might be corrupted (or the objects
missing due to a garbage collection), but that's no reason to truncate
the file and essentially destroy the rest of it.

Ideally missing objects should not cause a crash, we could just skip
them, but that's another patch.

Signed-off-by: Felipe Contreras 
---
 fast-import.c  |  7 +--
 t/t9300-fast-import.sh | 15 +++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 9fc7093..a975c34 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -329,6 +329,7 @@ static const char *export_marks_file;
 static const char *import_marks_file;
 static int import_marks_file_from_stream;
 static int import_marks_file_ignore_missing;
+static int import_marks_file_done;
 static int relative_marks_paths;
 
 /* Our last blob */
@@ -1802,7 +1803,7 @@ static void dump_marks(void)
static struct lock_file mark_lock;
FILE *f;
 
-   if (!export_marks_file)
+   if (!export_marks_file || (import_marks_file && 
!import_marks_file_done))
return;
 
if (hold_lock_file_for_update(_lock, export_marks_file, 0) < 0) {
@@ -1835,7 +1836,7 @@ static void read_marks(void)
if (f)
;
else if (import_marks_file_ignore_missing && errno == ENOENT)
-   return; /* Marks file does not exist */
+   goto done; /* Marks file does not exist */
else
die_errno("cannot read '%s'", import_marks_file);
while (fgets(line, sizeof(line), f)) {
@@ -1865,6 +1866,8 @@ static void read_marks(void)
insert_mark(mark, e);
}
fclose(f);
+done:
+   import_marks_file_done = 1;
 }
 
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 25bb60b..4bca35c 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2650,6 +2650,21 @@ test_expect_success 'R: ignore non-git options' '
git fast-import expect <<-EOF &&
+   :3 
+   :1 $blob
+   :2 $blob
+   EOF
+   cp expect io.marks &&
+   test_must_fail git fast-import --import-marks=io.marks 
--export-marks=io.marks <<-\EOF &&
+
+   EOF
+   test_cmp expect io.marks
+'
+
 ##
 ## R: very large blobs
 ##
-- 
2.8.2.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 v2 10/12] attr: convert git_all_attrs() to use "struct git_attr_check"

[Junio C Hamano]

Junio C Hamano  writes:

> Given that one of the two expected callers, namely, "check-attr" and
> Stefan's pathspec label magic, of this "alloc and then append" side
> of the API wants to have an access to git_attr(name), I think
> the function signature for this one should be updated to take not
> "const char *name" but instead take "struct git_attr *attr", i.e.

Perhaps this can be squashed into 12/12 to update the tutorial part
to cover this less often used form.

 Documentation/technical/api-gitattributes.txt | 22 +-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 6f13f94..92fc32a 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -55,7 +55,11 @@ Querying Specific Attributes
 
 * Prepare `struct git_attr_check` using git_attr_check_initl()
   function, enumerating the names of attributes whose values you are
-  interested in, terminated with a NULL pointer.
+  interested in, terminated with a NULL pointer.  Alternatively, an
+  empty `struct git_attr_check` can be prepared by calling
+  `git_attr_check_alloc()` function and then attributes you want to
+  ask about can be added to it with `git_attr_check_append()`
+  function.
 
 * Call `git_check_attr()` to check the attributes for the path.
 
@@ -112,6 +116,22 @@ static void setup_check(void)
}
 
 
+To see how attributes in argv[] are set for different paths, only
+the first step in the above would be different.
+
+
+static struct git_attr_check *check;
+static void setup_check(const char **argv)
+{
+   check = git_attr_check_alloc();
+   while (*argv) {
+   struct git_attr *attr = git_attr(*argv);
+   git_attr_check_append(check, attr);
+   argv++;
+   }
+}
+
+
 
 Querying All Attributes
 ---
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

On Tue, May 17, 2016 at 4:37 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&
>
> This is kosher POSIX, but I vaguely recall some shells had trouble
> with the SP between -C and "$dir" in the past.  Let's see if anybody
> screams; hopefully I am misremembering or buggy shells died out.

I also am bothered by a vague recollection of some issue (possibly
involving the internal space and lack of quotes around the entire
${...}), but couldn't remember nor find a reference to the specific
details. Perhaps someone reading the patch has a better memory than I.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Junio C Hamano]

Eric Sunshine  writes:

> + git ${dir:+-C "$dir"} rev-parse --$o >actual &&

This is kosher POSIX, but I vaguely recall some shells had trouble
with the SP between -C and "$dir" in the past.  Let's see if anybody
screams; hopefully I am misremembering or buggy shells died out.

Other than that, the entire series makes the script easier to grok.
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 v6 00/17] Port branch.c to use ref-filter's printing options

[Junio C Hamano]

Karthik Nayak  writes:

> Sorry for that.
> The only reason I haven't based it on 'master' is because it doesn't contain
> 'f307218'.
>
> ➔ git branch --contains=f307218
>   next
>   ref-filter

It is not clear from the above what your local ref-filter contains
beyond 'master', so it is not very useful to me when I am trying to
help you to avoid taking this topic hostage to all the topics in
'next' (if I queued this directly on 'next', I have to hold this
series until all the topics in 'next' graduates to 'master).

The series certainly would not apply to f307218 at all; it depends
on other stuff you have either in your local 'ref-filter' or 'next'.
It does not apply to the result of a merge of 'es/test-gpg-tags' topic 
into 'master', either, but the above does not make it clear what
else you are using from 'next' at all.

In any case, I think I managed to reduce the dependency on only
'es/test-gpg-tags' and 'master', and that is what I'll be queuing.
Please double check the patches in kn/ref-filter-branch-list topic
and also the merge of it into 'pu' for mismerges.

The difference between the result of merging the previously queued
one to 'pu', and the result of merging this round to 'pu', looks
like the attached.

Thanks.

 builtin/branch.c   |  2 +-
 ref-filter.c   |  6 --
 t/t6302-for-each-ref-filter.sh | 30 +++---
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0bbb4de..2412738 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -293,7 +293,7 @@ static int calc_maxwidth(struct ref_array *refs, int 
remote_bonus)
skip_prefix(it->refname, "refs/remotes/", );
if (it->kind == FILTER_REFS_DETACHED_HEAD) {
char *head_desc = get_head_description();
-   w = strlen(head_desc);
+   w = utf8_strwidth(head_desc);
free(head_desc);
} else
w = utf8_strwidth(desc);
diff --git a/ref-filter.c b/ref-filter.c
index 74c4869..f25671c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1196,12 +1196,14 @@ char *get_head_description(void)
strbuf_addf(, _("(no branch, bisect started on %s)"),
state.branch);
else if (state.detached_from) {
-   /* TRANSLATORS: make sure these match _("HEAD detached at ")
-  and _("HEAD detached from ") in wt-status.c */
if (state.detached_at)
+   /* TRANSLATORS: make sure this matches
+  "HEAD detached at " in wt-status.c */
strbuf_addf(, _("(HEAD detached at %s)"),
state.detached_from);
else
+   /* TRANSLATORS: make sure this matches
+  "HEAD detached from " in wt-status.c */
strbuf_addf(, _("(HEAD detached from %s)"),
state.detached_from);
}
diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
index 331d978..a09a1a4 100755
--- a/t/t6302-for-each-ref-filter.sh
+++ b/t/t6302-for-each-ref-filter.sh
@@ -342,22 +342,22 @@ test_expect_success 'improper usage of %(if), %(then), 
%(else) and %(end) atoms'
 '
 
 test_expect_success 'check %(if)...%(then)...%(end) atoms' '
-   git for-each-ref --format="%(if)%(authorname)%(then)%(authorname): 
%(refname)%(end)" >actual &&
+   git for-each-ref --format="%(refname)%(if)%(authorname)%(then) Author: 
%(authorname)%(end)" >actual &&
cat >expect <<-\EOF &&
-   A U Thor: refs/heads/master
-   A U Thor: refs/heads/side
-   A U Thor: refs/odd/spot
-
-
-
-   A U Thor: refs/tags/foo1.10
-   A U Thor: refs/tags/foo1.3
-   A U Thor: refs/tags/foo1.6
-   A U Thor: refs/tags/four
-   A U Thor: refs/tags/one
-
-   A U Thor: refs/tags/three
-   A U Thor: refs/tags/two
+   refs/heads/master Author: A U Thor
+   refs/heads/side Author: A U Thor
+   refs/odd/spot Author: A U Thor
+   refs/tags/annotated-tag
+   refs/tags/doubly-annotated-tag
+   refs/tags/doubly-signed-tag
+   refs/tags/foo1.10 Author: A U Thor
+   refs/tags/foo1.3 Author: A U Thor
+   refs/tags/foo1.6 Author: A U Thor
+   refs/tags/four Author: A U Thor
+   refs/tags/one Author: A U Thor
+   refs/tags/signed-tag
+   refs/tags/three Author: A U Thor
+   refs/tags/two Author: A U Thor
EOF
test_cmp expect actual
 '

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

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano  wrote:
>> When matching (i.e. the match_attrs() function), you would instead
>> do
>>
>> path = xmemdupz(name, namelen);
>> git_check_attr(path, item->attr_check);
>>
>> to grab values for only attributes that matter to you, instead of
>> calling git_all_attrs() [*2*].
>>
>> After git_check_attr() returns, item->attr_check.check[0].attr would
>> be git_attr("VAR1") and item->attr_check.check[0].value would be
>> whatever setting the path has for the VAR1 attribute.  You can use
>> your match_mode logic to compare it with the values .attr_match
>> expects.
>>
>> You do not necessarily have to have the same number of elements in
>> .attr_match and .attr_check.check by the way.  .attr_match might say
>>
>> VAR1=VAL2 !VAR1 -VAR1
>>
>> which may be always false if these are ANDed together, but in order
>> to evaluate it, you need only one git_attr_check_elem for VAR1.
>

The key phrase in the message you are reacting to is "not
necessarily".  It is not a crime to ask for the same attribute twice
in a git_attr_check structure.

$ git check-attr text text text -- path

would stuff three instances of "text" in there and ask them for
"path".  The simple in-code callers that uses git_attr_check_initl()
do rely on the order of the attributes it placed in attr_check
structure (see e.g. how ll_merge() uses check[0].value and
check[1].value to see the driver name and marker size), and that is
perfectly kosher.  Existing code is your friend.

The mention of the possibility is purely as a hint useful for a
possible enhancement in the far future.  If we ever want to support
something like this:

":(attr-expression (VAR1=VAL1 | VAR1=VAL2) & VAR2)"

you can remember that you can put VAR1 and VAR2 in attr_check to
grab values for VAR1 and VAR2 (even though VAR1 is mentioned twice
in the expression), and use them in the evaluation you will perform.

> So for the matching we would need to get the order right, i.e.
>
> const char *inspect_name = git_attr_name(item.attr_match[i].attr);
> for (j=0; j <  p.attr_check.check_nr; j++) {
> const char *cur_name = git_attr_name(p.attr_check.check[j].attr);
> if (!strcmp(inspect_name, cur_name))
> break;

You do not strcmp() when you have attributes.  They are interned so
that you can compare their addresses.  That makes it somewhat
cheaper.

Once you start "expression over random attributes", you'd need to
map attr => value somehow.  The format attr_check structure gives
you, i.e. a list of , is aimed at compactness than
random hashmap-like access.  If the caller wants a hashmap-like
access for performance purposes, the caller does that itself.

Existing users do not need a hashmap-like access, because they know
at which index in attr_check they placed request for what attribute.
An array that can be indexed with a small integer is exactly what
they want.

> This doesn't look cheap to me? Am I holding it wrong again?

By the way, I do not think during the entire discussion on this
topic, you have never been in a situation to deserve the "holding it
wrong" label (which implies "a ware is broken, but somehow the end
user is blamed for using it incorrectly").  When you were wrong, you
were simply wrong.
--
To unsubscribe from this list: send the line "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] A part of an edge from an octopus merge gets colored, even with --color=never

[Johannes Sixt]

Am 17.05.2016 um 21:45 schrieb Jeff King:

On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote:


Am 15.05.2016 um 15:05 schrieb Noam Postavsky:

With a certain topology involving an octopus merge, git log --graph
--oneline --all --color=never produces output which includes some ANSI
escape code coloring. Attached is a script to reproduce the problem
(creates a git repository in subdir log-format-test), along with
sample graph and valgrind output (indicates some unitialialized memory
access).

I've observed the problem with Windows git versions 2.7.0, 2.5.3.
I've NOT observed it with 1.9.5,

On GNU/Linux the symptom only appears when running with valgrind, I
tried versions
2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)



Sorry, I can't reproduce your observation. I ran the script you provided
with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.


Interesting. It replicates out of the box for me.


"Out of the box" are the magic words. I usually compile with -O0, which 
doesn't trigger the valgrind report.


When I compile with a 3.x based gcc on Windows, I see these warnings:

CC color.o
color.c: In function 'color_parse_mem':
color.c:203: warning: 'c.value' may be used uninitialized in this function
color.c:203: warning: 'c.blue' may be used uninitialized in this function
color.c:203: warning: 'c.green' may be used uninitialized in this function
color.c:203: warning: 'c.red' may be used uninitialized in this function

(which triggered my curiosity in this bug report). But they seem to be 
unrelated and are most likely false positives.


-- 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: [PATCH v6 05/17] ref-filter: move get_head_description() from branch.c

[Junio C Hamano]

Karthik Nayak  writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 2ecde53..141168d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -361,39 +361,6 @@ static void add_verbose_info(struct strbuf *out, struct 
> ref_array_item *item,
>   strbuf_release();
>  }
>  
> -static char *get_head_description(void)
> -{
> - struct strbuf desc = STRBUF_INIT;
> - struct wt_status_state state;
> - memset(, 0, sizeof(state));
> - wt_status_get_state(, 1);
> - if (state.rebase_in_progress ||
> - state.rebase_interactive_in_progress)
> - strbuf_addf(, _("(no branch, rebasing %s)"),
> - state.branch);
> - else if (state.bisect_in_progress)
> - strbuf_addf(, _("(no branch, bisect started on %s)"),
> - state.branch);
> - else if (state.detached_from) {
> - if (state.detached_at)
> - /* TRANSLATORS: make sure this matches
> -"HEAD detached at " in wt-status.c */
> - strbuf_addf(, _("(HEAD detached at %s)"),
> - state.detached_from);
> - else
> - /* TRANSLATORS: make sure this matches
> -"HEAD detached from " in wt-status.c */
> - strbuf_addf(, _("(HEAD detached from %s)"),
> - state.detached_from);
> - }

Note that this expects that va/i18n-misc-updates topic, which
corrects the translator instruction around here, is already applied.

> diff --git a/ref-filter.c b/ref-filter.c
> index 7d3af1c..fcb3353 100644
> ...
> +char *get_head_description(void)
> +{
> + struct strbuf desc = STRBUF_INIT;
> + struct wt_status_state state;
> + memset(, 0, sizeof(state));
> + wt_status_get_state(, 1);
> + if (state.rebase_in_progress ||
> + state.rebase_interactive_in_progress)
> + strbuf_addf(, _("(no branch, rebasing %s)"),
> + state.branch);
> + else if (state.bisect_in_progress)
> + strbuf_addf(, _("(no branch, bisect started on %s)"),
> + state.branch);
> + else if (state.detached_from) {
> + /* TRANSLATORS: make sure these match _("HEAD detached at ")
> +and _("HEAD detached from ") in wt-status.c */
> + if (state.detached_at)
> + strbuf_addf(, _("(HEAD detached at %s)"),
> + state.detached_from);
> + else
> + strbuf_addf(, _("(HEAD detached from %s)"),
> + state.detached_from);
> + }

... but the change is apparently lost.

It is a good lesson not to blindly rebase things on 'next', which
would have unrelated changes.  If you needed es/test-gpg-tags topic
for the test script change, check out 'master', merge that single
topic, and then rebase the series on top of the result.

--
To unsubscribe from this list: send the line "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] A part of an edge from an octopus merge gets colored, even with --color=never

[Jeff King]

On Tue, May 17, 2016 at 03:51:37PM -0400, Jeff King wrote:

> On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote:
> 
> > Note that we set up f90, fa0, and fb0, but then pass fc0 into
> > strbuf_write_column (and it has bogus color values). It looks like we're
> > reading one past the end of our array, but I haven't figured out where
> > or why.
> 
> Looking at the valgrind output reveals that. Here's an assert() that
> catches it reliably for me:
> 
> diff --git a/graph.c b/graph.c
> index 1350bdd..964bbd1 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph 
> *graph,
>   ((graph->num_parents - dashless_commits) * 2) - 1;
>   for (i = 0; i < num_dashes; i++) {
>   col_num = (i / 2) + dashless_commits + graph->commit_index;
> + assert(col_num < graph->num_new_columns);
>   strbuf_write_column(sb, >new_columns[col_num], '-');
>   }
>   col_num = (i / 2) + dashless_commits + graph->commit_index;
> + assert(col_num < graph->num_new_columns);
>   strbuf_write_column(sb, >new_columns[col_num], '.');
>   return num_dashes + 1;
>  }
> 
> (It's actually the first one which triggers). I'm not familiar enough
> with the code to know whether the col_num computation is bogus, or
> whether we needed to earlier increase the size of the "new_columns"
> field.

And unsurprisingly, reverting 339c17bc7690b5436ac61c996cede3d52c85b50d
seems to fix this (author cc'd). It's the extra "commit_index" addition
that causes the problem. But I'm still not sure what the correct
solution is.

-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: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

[Jeff King]

On Tue, May 17, 2016 at 03:45:34PM -0400, Jeff King wrote:

> Note that we set up f90, fa0, and fb0, but then pass fc0 into
> strbuf_write_column (and it has bogus color values). It looks like we're
> reading one past the end of our array, but I haven't figured out where
> or why.

Looking at the valgrind output reveals that. Here's an assert() that
catches it reliably for me:

diff --git a/graph.c b/graph.c
index 1350bdd..964bbd1 100644
--- a/graph.c
+++ b/graph.c
@@ -794,9 +794,11 @@ static int graph_draw_octopus_merge(struct git_graph 
*graph,
((graph->num_parents - dashless_commits) * 2) - 1;
for (i = 0; i < num_dashes; i++) {
col_num = (i / 2) + dashless_commits + graph->commit_index;
+   assert(col_num < graph->num_new_columns);
strbuf_write_column(sb, >new_columns[col_num], '-');
}
col_num = (i / 2) + dashless_commits + graph->commit_index;
+   assert(col_num < graph->num_new_columns);
strbuf_write_column(sb, >new_columns[col_num], '.');
return num_dashes + 1;
 }

(It's actually the first one which triggers). I'm not familiar enough
with the code to know whether the col_num computation is bogus, or
whether we needed to earlier increase the size of the "new_columns"
field.

-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: [BUG] A part of an edge from an octopus merge gets colored, even with --color=never

[Jeff King]

On Tue, May 17, 2016 at 09:07:33PM +0200, Johannes Sixt wrote:

> Am 15.05.2016 um 15:05 schrieb Noam Postavsky:
> > With a certain topology involving an octopus merge, git log --graph
> > --oneline --all --color=never produces output which includes some ANSI
> > escape code coloring. Attached is a script to reproduce the problem
> > (creates a git repository in subdir log-format-test), along with
> > sample graph and valgrind output (indicates some unitialialized memory
> > access).
> > 
> > I've observed the problem with Windows git versions 2.7.0, 2.5.3.
> > I've NOT observed it with 1.9.5,
> > 
> > On GNU/Linux the symptom only appears when running with valgrind, I
> > tried versions
> > 2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes 
> > from)
> > 
> 
> Sorry, I can't reproduce your observation. I ran the script you provided
> with HOME=$PWD and a minimal .gitconfig that only sets user.email. But
> valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.

Interesting. It replicates out of the box for me. It looks like the
column pointer we are passing is bogus. If I instrument git like this:

diff --git a/graph.c b/graph.c
index 1350bdd..62a5810 100644
--- a/graph.c
+++ b/graph.c
@@ -76,6 +76,7 @@ static const char *column_get_color_code(unsigned short color)
 static void strbuf_write_column(struct strbuf *sb, const struct column *c,
char col_char)
 {
+   warning("c=%p, c->color = %d, max=%d", c, c->color, column_colors_max);
if (c->color < column_colors_max)
strbuf_addstr(sb, column_get_color_code(c->color));
strbuf_addch(sb, col_char);
@@ -390,6 +391,9 @@ static void graph_insert_into_new_columns(struct git_graph 
*graph,
 */
graph->new_columns[graph->num_new_columns].commit = commit;
graph->new_columns[graph->num_new_columns].color = 
graph_find_commit_color(graph, commit);
+   warning("assigned %p, %d",
+   >new_columns[graph->num_new_columns],
+   graph->new_columns[graph->num_new_columns].color);
graph->mapping[*mapping_index] = graph->num_new_columns;
*mapping_index += 2;
graph->num_new_columns++;

Then I get this output:

warning: assigned 0x20d21d0, 12
* 163b784 (c) c
warning: assigned 0x20d8f90, 12
warning: assigned 0x20d8fa0, 12
warning: assigned 0x20d8fb0, 12
warning: c=0x20d21d0, c->color = 12, max=12
warning: c=0x20d8fc0, c->color = 0, max=12
warning: c=0x20d8fc0, c->color = 0, max=12
| *-.   a9a6975 (HEAD -> m) merge a b

Note that we set up f90, fa0, and fb0, but then pass fc0 into
strbuf_write_column (and it has bogus color values). It looks like we're
reading one past the end of our array, but I haven't figured out where
or why.

-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

[PATCH v2 0/5] modernize t1500

[Eric Sunshine]

This is a re-roll of [1] which modernizes t1500 by updating tests to set
up their own needed state rather than relying upon manipulation of
global state.

Changes since v1[1]:

In v1 patch 1/6, which makes test_rev_parse() for-loop driven, the loop
control has been moved to the top of the loop for improved robustness.

v1 patch 2/6, which tweaked the value of GIT_CONFIG in preparation for
removal of global cd's has been squashed into patch 3/6 which actually
removes the cd's since the below diff makes perfect sense in the context
of the latter patch, thus doesn't require its own preparatory patch.

-cd work || exit 1
-GIT_CONFIG="$(pwd)"/../.git/config
+GIT_CONFIG="$(pwd)/work/../.git/config"

v1 patch 3/6, which teaches test_rev_parse() the -C option, has been
revised to ensure that the argument to -C is always quoted upon use to
avoid problems with whitespace in directory names (though current tests
don't have any such directories).

v1 patches 4/6 and 5/6, which teach test_rev_parse() the -b and -g
options, no longer assigns shell code to a variable for later
execution/evaluation; instead they just execute the code directly.

v1 patch 5/6 ensures that the argument to -g is properly quoted when
assigned to GIT_DIR to avoid problems with whitespace in directory
names.

v1 patch 6/6, which changes "mv .git repo.git" to "cp -R .git repo.git",
has been squashed with Junio's 7/6[2], which moves the "cp -R" earlier
in the script, and now sits at the front of the series. Other
setup-related actions have likewise been moved into a common "setup"
test[3].

Most commit messages have seen minor tweaks.

A v1 to v2 interdiff is included below.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/294088
[2]: http://thread.gmane.org/gmane.comp.version-control.git/294088/focus=294168
[3]: http://thread.gmane.org/gmane.comp.version-control.git/294088/focus=294170

Eric Sunshine (5):
  t1500: be considerate to future potential tests
  t1500: test_rev_parse: facilitate future test enhancements
  t1500: avoid changing working directory outside of tests
  t1500: avoid setting configuration options outside of tests
  t1500: avoid setting environment variables outside of tests

 t/t1500-rev-parse.sh | 123 ++-
 1 file changed, 63 insertions(+), 60 deletions(-)

--- >8 ---
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index af223ed..39af565 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -7,26 +7,21 @@ test_description='test git rev-parse'
 test_rev_parse () {
dir=
bare=
-   env=
+   gitdir=
while :
do
case "$1" in
-   -C) dir="-C $2"; shift; shift ;;
-   -b) bare="$2"; shift; shift ;;
-   -g) env="GIT_DIR=$2; export GIT_DIR"; shift; shift ;;
+   -C) dir="$2"; shift; shift ;;
+   -b) case "$2" in
+   [tfu]*) bare="$2"; shift; shift ;;
+   *) error "test_rev_parse: bogus core.bare value '$2'" ;;
+   esac ;;
+   -g) gitdir="$2"; shift; shift ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
done
 
-   case "$bare" in
-   '') ;;
-   t*) bare="test_config $dir core.bare true" ;;
-   f*) bare="test_config $dir core.bare false" ;;
-   u*) bare="test_unconfig $dir core.bare" ;;
-   *) error "test_rev_parse: unrecognized core.bare value '$bare'"
-   esac
-
name=$1
shift
 
@@ -36,35 +31,48 @@ test_rev_parse () {
 show-prefix \
 git-dir
do
+   test $# -eq 0 && break
expect="$1"
test_expect_success "$name: $o" '
-   test_when_finished "sane_unset GIT_DIR" &&
-   eval $env &&
-   $bare &&
+   if test -n "$gitdir"
+   then
+   test_when_finished "unset GIT_DIR" &&
+   GIT_DIR="$gitdir" &&
+   export GIT_DIR
+   fi &&
+
+   case "$bare" in
+   t*) test_config ${dir:+-C "$dir"} core.bare true ;;
+   f*) test_config ${dir:+-C "$dir"} core.bare false ;;
+   u*) test_unconfig ${dir:+-C "$dir"} core.bare ;;
+   esac &&
+
echo "$expect" >expect &&
-   git $dir rev-parse --$o >actual &&
+   git ${dir:+-C "$dir"} rev-parse --$o >actual &&
test_cmp expect actual
'
shift
-   test $# -eq 0 && break
done
 }
 
 ROOT=$(pwd)
 
+test_expect_success 'setup' '
+   mkdir -p sub/dir work &&
+   cp -R .git repo.git
+'
+
 test_rev_parse toplevel 

[PATCH v2 3/5] t1500: avoid changing working directory outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-C " option to allow callers to
instruct it explicitly in which directory its tests should be run. Take
advantage of this new option to avoid changing the working directory
outside of tests.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 42 +++---
 1 file changed, 23 insertions(+), 19 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index c84f5c3..fb2cee8 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,8 +3,18 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
+   dir=
+   while :
+   do
+   case "$1" in
+   -C) dir="$2"; shift; shift ;;
+   -*) error "test_rev_parse: unrecognized option '$1'" ;;
+   *) break ;;
+   esac
+   done
+
name=$1
shift
 
@@ -18,7 +28,7 @@ test_rev_parse () {
expect="$1"
test_expect_success "$name: $o" '
echo "$expect" >expect &&
-   git rev-parse --$o >actual &&
+   git ${dir:+-C "$dir"} rev-parse --$o >actual &&
test_cmp expect actual
'
shift
@@ -34,15 +44,10 @@ test_expect_success 'setup' '
 
 test_rev_parse toplevel false false true '' .git
 
-cd .git || exit 1
-test_rev_parse .git/ false true false '' .
-cd objects || exit 1
-test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse -C .git .git/ false true false '' .
+test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
 
-cd sub/dir || exit 1
-test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
-cd ../.. || exit 1
+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
 git config core.bare true
 test_rev_parse 'core.bare = true' true false false
@@ -50,30 +55,29 @@ test_rev_parse 'core.bare = true' true false false
 git config --unset core.bare
 test_rev_parse 'core.bare undefined' false false true
 
-cd work || exit 1
 GIT_DIR=../.git
-GIT_CONFIG="$(pwd)"/../.git/config
+GIT_CONFIG="$(pwd)/work/../.git/config"
 export GIT_DIR GIT_CONFIG
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true 
''
 
 GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)"/../repo.git/config
+GIT_CONFIG="$(pwd)/work/../repo.git/config"
 
 git config core.bare false
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false 
true ''
 
 git config core.bare true
-test_rev_parse 'GIT_DIR=../repo.git, core.bare = true' true false false ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
 
 git config --unset core.bare
-test_rev_parse 'GIT_DIR=../repo.git, core.bare undefined' false false true ''
+test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false 
true ''
 
 test_done
-- 
2.8.2.703.g78b384c

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

[PATCH v2 2/5] t1500: test_rev_parse: facilitate future test enhancements

[Eric Sunshine]

Tests run by test_rev_parse() are nearly identical; each invokes
git-rev-parse with a single option and compares the result against an
expected value. Such duplication makes it onerous to extend the tests
since any change needs to be repeated in each test. Avoid the
duplication by parameterizing the test and driving it via a for-loop.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 44 +---
 1 file changed, 17 insertions(+), 27 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 0194f54..c84f5c3 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,38 +3,28 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-test_rev_parse() {
+# usage: label is-bare is-inside-git is-inside-work prefix git-dir
+test_rev_parse () {
name=$1
shift
 
-   test_expect_success "$name: is-bare-repository" \
-   "test '$1' = \"\$(git rev-parse --is-bare-repository)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: is-inside-git-dir" \
-   "test '$1' = \"\$(git rev-parse --is-inside-git-dir)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: is-inside-work-tree" \
-   "test '$1' = \"\$(git rev-parse --is-inside-work-tree)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: prefix" \
-   "test '$1' = \"\$(git rev-parse --show-prefix)\""
-   shift
-   [ $# -eq 0 ] && return
-
-   test_expect_success "$name: git-dir" \
-   "test '$1' = \"\$(git rev-parse --git-dir)\""
-   shift
-   [ $# -eq 0 ] && return
+   for o in is-bare-repository \
+is-inside-git-dir \
+is-inside-work-tree \
+show-prefix \
+git-dir
+   do
+   test $# -eq 0 && break
+   expect="$1"
+   test_expect_success "$name: $o" '
+   echo "$expect" >expect &&
+   git rev-parse --$o >actual &&
+   test_cmp expect actual
+   '
+   shift
+   done
 }
 
-# label is-bare is-inside-git is-inside-work prefix git-dir
-
 ROOT=$(pwd)
 
 test_expect_success 'setup' '
-- 
2.8.2.703.g78b384c

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

[PATCH v2 4/5] t1500: avoid setting configuration options outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-b " option to allow callers to set
"core.bare" explicitly or undefine it. Take advantage of this new option
to avoid setting "core.bare" outside of tests.

Under the hood, "-b " invokes "test_config -C " (or
"test_unconfig -C "), thus git-config knows explicitly where to
find its configuration file. Consequently, the global GIT_CONFIG
environment variable required by the manual git-config invocations
outside of tests is no longer needed, and is thus dropped.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 39 ---
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index fb2cee8..5be463f 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -6,10 +6,15 @@ test_description='test git rev-parse'
 # usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
 test_rev_parse () {
dir=
+   bare=
while :
do
case "$1" in
-C) dir="$2"; shift; shift ;;
+   -b) case "$2" in
+   [tfu]*) bare="$2"; shift; shift ;;
+   *) error "test_rev_parse: bogus core.bare value '$2'" ;;
+   esac ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
@@ -27,6 +32,12 @@ test_rev_parse () {
test $# -eq 0 && break
expect="$1"
test_expect_success "$name: $o" '
+   case "$bare" in
+   t*) test_config ${dir:+-C "$dir"} core.bare true ;;
+   f*) test_config ${dir:+-C "$dir"} core.bare false ;;
+   u*) test_unconfig ${dir:+-C "$dir"} core.bare ;;
+   esac &&
+
echo "$expect" >expect &&
git ${dir:+-C "$dir"} rev-parse --$o >actual &&
test_cmp expect actual
@@ -49,35 +60,25 @@ test_rev_parse -C .git/objects .git/objects/ false true 
false '' "$ROOT/.git"
 
 test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
 
-git config core.bare true
-test_rev_parse 'core.bare = true' true false false
+test_rev_parse -b t 'core.bare = true' true false false
 
-git config --unset core.bare
-test_rev_parse 'core.bare undefined' false false true
+test_rev_parse -b u 'core.bare undefined' false false true
 
 GIT_DIR=../.git
-GIT_CONFIG="$(pwd)/work/../.git/config"
-export GIT_DIR GIT_CONFIG
+export GIT_DIR
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false 
true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare = true' true false false ''
+test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false 
false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../.git, core.bare undefined' false false true 
''
+test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false 
true ''
 
 GIT_DIR=../repo.git
-GIT_CONFIG="$(pwd)/work/../repo.git/config"
 
-git config core.bare false
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = false' false false 
true ''
+test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false 
false true ''
 
-git config core.bare true
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
 
-git config --unset core.bare
-test_rev_parse -C work 'GIT_DIR=../repo.git, core.bare undefined' false false 
true ''
+test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false 
false true ''
 
 test_done
-- 
2.8.2.703.g78b384c

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

[PATCH v2 1/5] t1500: be considerate to future potential tests

[Eric Sunshine]

The final batch of git-rev-parse tests work against a non-local object
database named repo.git. This is done by renaming .git to repo.git and
pointing GIT_DIR at it, but the name is never restored to .git at the
end of the script, which can be problematic for tests added in the
future. Be more friendly by instead making repo.git a copy of .git.

Furthermore, make it clear that tests in repo.git will be independent
from the results of earlier tests done in .git by initializing repo.git
earlier in the test sequence.

Likewise, bundle remaining preparation (such as directory creation) into
a common setup test consistent with modern practice.

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 48ee077..0194f54 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -37,6 +37,11 @@ test_rev_parse() {
 
 ROOT=$(pwd)
 
+test_expect_success 'setup' '
+   mkdir -p sub/dir work &&
+   cp -R .git repo.git
+'
+
 test_rev_parse toplevel false false true '' .git
 
 cd .git || exit 1
@@ -45,7 +50,6 @@ cd objects || exit 1
 test_rev_parse .git/objects/ false true false '' "$ROOT/.git"
 cd ../.. || exit 1
 
-mkdir -p sub/dir || exit 1
 cd sub/dir || exit 1
 test_rev_parse subdirectory false false true sub/dir/ "$ROOT/.git"
 cd ../.. || exit 1
@@ -56,7 +60,6 @@ test_rev_parse 'core.bare = true' true false false
 git config --unset core.bare
 test_rev_parse 'core.bare undefined' false false true
 
-mkdir work || exit 1
 cd work || exit 1
 GIT_DIR=../.git
 GIT_CONFIG="$(pwd)"/../.git/config
@@ -71,7 +74,6 @@ test_rev_parse 'GIT_DIR=../.git, core.bare = true' true false 
false ''
 git config --unset core.bare
 test_rev_parse 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
-mv ../.git ../repo.git || exit 1
 GIT_DIR=../repo.git
 GIT_CONFIG="$(pwd)"/../repo.git/config
 
-- 
2.8.2.703.g78b384c

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

[PATCH v2 5/5] t1500: avoid setting environment variables outside of tests

[Eric Sunshine]

Ideally, each test should be responsible for setting up state it needs
rather than relying upon transient global state. Toward this end, teach
test_rev_parse() to accept a "-g " option to allow callers to
specify the value of the GIT_DIR environment variable explicitly. Take
advantage of this new option to avoid polluting the global scope with
GIT_DIR assignments.

Implementation note: Typically, tests avoid polluting the global state
by wrapping transient environment variable assignments within a
subshell, however, this technique doesn't work here since test_config()
and test_unconfig() need to know GIT_DIR, as well, but neither function
can be used within a subshell. Consequently, GIT_DIR is instead cleared
manually via test_when_finished().

Signed-off-by: Eric Sunshine 
---
 t/t1500-rev-parse.sh | 24 +++-
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 5be463f..39af565 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -7,6 +7,7 @@ test_description='test git rev-parse'
 test_rev_parse () {
dir=
bare=
+   gitdir=
while :
do
case "$1" in
@@ -15,6 +16,7 @@ test_rev_parse () {
[tfu]*) bare="$2"; shift; shift ;;
*) error "test_rev_parse: bogus core.bare value '$2'" ;;
esac ;;
+   -g) gitdir="$2"; shift; shift ;;
-*) error "test_rev_parse: unrecognized option '$1'" ;;
*) break ;;
esac
@@ -32,6 +34,13 @@ test_rev_parse () {
test $# -eq 0 && break
expect="$1"
test_expect_success "$name: $o" '
+   if test -n "$gitdir"
+   then
+   test_when_finished "unset GIT_DIR" &&
+   GIT_DIR="$gitdir" &&
+   export GIT_DIR
+   fi &&
+
case "$bare" in
t*) test_config ${dir:+-C "$dir"} core.bare true ;;
f*) test_config ${dir:+-C "$dir"} core.bare false ;;
@@ -64,21 +73,18 @@ test_rev_parse -b t 'core.bare = true' true false false
 
 test_rev_parse -b u 'core.bare undefined' false false true
 
-GIT_DIR=../.git
-export GIT_DIR
 
-test_rev_parse -C work -b f 'GIT_DIR=../.git, core.bare = false' false false 
true ''
+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' 
false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' 
true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../.git, core.bare undefined' false false 
true ''
+test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' 
false false true ''
 
-GIT_DIR=../repo.git
 
-test_rev_parse -C work -b f 'GIT_DIR=../repo.git, core.bare = false' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = 
false' false false true ''
 
-test_rev_parse -C work -b t 'GIT_DIR=../repo.git, core.bare = true' true false 
false ''
+test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = 
true' true false false ''
 
-test_rev_parse -C work -b u 'GIT_DIR=../repo.git, core.bare undefined' false 
false true ''
+test_rev_parse -C work -g ../repo.git -b u 'GIT_DIR=../repo.git, core.bare 
undefined' false false true ''
 
 test_done
-- 
2.8.2.703.g78b384c

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

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano  wrote:
> When matching (i.e. the match_attrs() function), you would instead
> do
>
> path = xmemdupz(name, namelen);
> git_check_attr(path, item->attr_check);
>
> to grab values for only attributes that matter to you, instead of
> calling git_all_attrs() [*2*].
>
> After git_check_attr() returns, item->attr_check.check[0].attr would
> be git_attr("VAR1") and item->attr_check.check[0].value would be
> whatever setting the path has for the VAR1 attribute.  You can use
> your match_mode logic to compare it with the values .attr_match
> expects.
>
> You do not necessarily have to have the same number of elements in
> .attr_match and .attr_check.check by the way.  .attr_match might say
>
> VAR1=VAL2 !VAR1 -VAR1
>
> which may be always false if these are ANDed together, but in order
> to evaluate it, you need only one git_attr_check_elem for VAR1.

So for the matching we would need to get the order right, i.e.

const char *inspect_name = git_attr_name(item.attr_match[i].attr);
for (j=0; j <  p.attr_check.check_nr; j++) {
const char *cur_name = git_attr_name(p.attr_check.check[j].attr);
if (!strcmp(inspect_name, cur_name))
break;
// now compare .attr_match[i] with attr_check.check[j]

This doesn't look cheap to me? Am I holding it wrong again?
--
To unsubscribe from this list: send the line "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] A part of an edge from an octopus merge gets colored, even with --color=never

[Johannes Sixt]

Am 15.05.2016 um 15:05 schrieb Noam Postavsky:

With a certain topology involving an octopus merge, git log --graph
--oneline --all --color=never produces output which includes some ANSI
escape code coloring. Attached is a script to reproduce the problem
(creates a git repository in subdir log-format-test), along with
sample graph and valgrind output (indicates some unitialialized memory
access).

I've observed the problem with Windows git versions 2.7.0, 2.5.3.
I've NOT observed it with 1.9.5,

On GNU/Linux the symptom only appears when running with valgrind, I
tried versions
2.8.0, and 2.8.2.402.gedec370 (the last is where the valgrind output comes from)



Sorry, I can't reproduce your observation. I ran the script you provided 
with HOME=$PWD and a minimal .gitconfig that only sets user.email. But 
valgrind is happy with both 2.8.0 and v2.8.2-402-gedec370 on my Linux box.


-- 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: [PATCH v6 00/17] Port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Tue, May 17, 2016 at 11:22 PM, Junio C Hamano  wrote:
> Karthik Nayak  writes:
>
>> Hello, sorry for the confusion, it's built on top of 'next' which contains
>> f307218 (t6302: simplify non-gpg cases). The merge conflict is due to the
>> commit made by you 1cca17df (Documentation: fix linkgit references).
>
> That is not "confusion", but an "incorrect piece of information".
>
> The series does not seem to apply on 'next', either.
>
> Where did you exactly rebase on top of?  It is not on f307218, it is
> not on 'next', 'next@{1}',... 'next@{8}'.
>
> f3072180 (t6302: simplify non-gpg cases, 2016-05-09) was merged to
> 'next' at 9fcb98b2 (Merge branch 'es/test-gpg-tags' into next,
> 2016-05-10), but the series does not seem to apply there, either.
>
> $ git co 9fcb98b2
> Applying: ref-filter: implement %(if), %(then), and %(else) atoms
> error: patch failed: Documentation/git-for-each-ref.txt:181
> error: Documentation/git-for-each-ref.txt: patch does not apply
> Patch failed at 0001 ref-filter: implement %(if), %(then), and %(else) atoms
> The copy of the patch that failed is found in: .git/rebase-apply/patch
> When you have resolved this problem, run "git am --continue".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>
> Not that a series built on top of any 'next' is directly usable.
> You are forcing me to identify which topics in 'next' you depend on,
> and build a topic that does not contain anything unrelated that is
> in 'next' before starting to apply these patches.  Can you pick a
> more appropriate place to base these patches on, please?  Why isn't
> this based on 'master', for example?

Hello,

Sorry for that.
The only reason I haven't based it on 'master' is because it doesn't contain
'f307218'.

➔ git branch --contains=f307218
  next
  ref-filter

Now speaking of which, this is based on next.

➔ git branch -v
* next   78b384c Sync with master

And Idk what the problem is but it seems to apply perfectly on top of it [1]

➔ git am v6-00*
Applying: ref-filter: implement %(if), %(then), and %(else) atoms
Applying: ref-filter: include reference to 'used_atom' within 'atom_value'
Applying: ref-filter: implement %(if:equals=) and
%(if:notequals=)
Applying: ref-filter: modify "%(objectname:short)" to take length
Applying: ref-filter: move get_head_description() from branch.c
Applying: ref-filter: introduce format_ref_array_item()
Applying: ref-filter: make %(upstream:track) prints "[gone]" for
invalid upstreams
Applying: ref-filter: add support for %(upstream:track,nobracket)
Applying: ref-filter: make "%(symref)" atom work with the ':short' modifier
Applying: ref-filter: introduce refname_atom_parser_internal()
Applying: ref-filter: introduce symref_atom_parser() and refname_atom_parser()
Applying: ref-filter: make remote_ref_atom_parser() use
refname_atom_parser_internal()
Applying: ref-filter: add `:dir` and `:base` options for ref printing atoms
Applying: ref-filter: allow porcelain to translate messages in the output
Applying: branch, tag: use porcelain output
Applying: branch: use ref-filter printing APIs
Applying: branch: implement '--format' option

[1] : https://github.com/KarthikNayak/git/commits/ref-filter

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "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/2] convert: ce_compare_data() checks for a sha1 of a path

[Junio C Hamano]

tbo...@web.de writes:

>  #define HASH_WRITE_OBJECT 1
>  #define HASH_FORMAT_CHECK 2
> +#define HASH_CE_HAS_SHA1  4

How does one pronounce the words in this constant?  Does it make a
listener understand what this constant means?


/*
 * We need a comment around here to say what these two
 * parameters mean.  I am guessing that (1) if sha1 is not NULL,
 * path is ignored and the function inspects if it has CR; (2)
 * otherwise it checks the index entry at path and inspects if
 * it has CR.
 */
>  
> -static int has_cr_in_index(const char *path)
> +static int has_cr_in_index(const char *path, const unsigned char *sha1)
>  {

This makes me seriously wonder if it is a good idea to modify this
function like this.  (1) means this function is not about IN INDEX
at all!

Perhaps add a "static int blob_has_cr(const unsigned char *sha1)"
and call it from the real caller you added that wants to call this
butchered two-param version that has sha1 is a better solution?

> -static int crlf_to_git(const char *path, const char *src, size_t len,
> +static int crlf_to_git(const char *path, const unsigned char *sha1,
> +const char *src, size_t len,
>  struct strbuf *buf,
>  enum crlf_action crlf_action, enum safe_crlf checksafe)
>  {
> @@ -260,7 +267,7 @@ 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, sha1))

I think this change is too ugly.  The new "sha1" parameter is
telling us that "in order to see if the indexed version has CR, do
not look at the index, but look at the contents of this blob".

But wouldn't the result become more understandable if instead we
passed a new parameter "cr-state-for-conversion" that takes UNKNOWN,
HAS_CR, or NO_CR to this function?

In other words, what if, when the caller knows it does not care
what's in the index but wants to instead see if a different blob has
CR, we make it the caller's responsibility to figure it out by
calling blob_has_cr() before calling into this codepath and pass the
result of that check down?  When cr-state-for-conversion is UNKNOWN,
this code knows that it needs to call has_cr_in_index(path) to
figure it out itself.  Otherwise, it can and should use the
caller-supplied value without asking has_cr_in_index(path).
--
To unsubscribe from this list: send the line "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/1] t6038; use crlf on all platforms

[Junio C Hamano]

tbo...@web.de writes:

> From: Torsten Bögershausen 
>
> t6038 uses different code, depending if NATIVE_CRLF is set ot not.
> When the native line endings are LF, merge.renormalize is not tested very 
> well.
> Change the test to always use CRLF by setting core.eol=crlf.
> ---
> Broke the 10/10 series into smaller pieces, this is the update of t6038

Thanks.

 - Missing sign-off.

 - "... is not tested very well", which implies "with this change,
   it will be tested", is a secondary benefit.  The reader need to
   agree that, whether the platform native line ending is CRLF or
   LF, 'git merge' should behave identically on any platform, as
   long as the line ending convention used in the repository is
   explicitly set in the same way, before agreeing that this is a
   good thing to do in general.  And that is a bigger benefit, no?

 - But doesn't the same principle apply in the other direction?
   When forced to do core.eol=lf, a platform with NATIVE_CRLF should
   behave identically to how a platform without NATIVE_CRLF would?

   With this patch, we lose test coverage for core.eol=lf case,
   which used to be tested on non-NATIVE_CRLF platforms.  Isn't that
   a concern to us?

>  t/t6038-merge-text-auto.sh | 37 +++--
>  1 file changed, 11 insertions(+), 26 deletions(-)
>
> diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
> index 85c10b0..4dc8c1a 100755
> --- a/t/t6038-merge-text-auto.sh
> +++ b/t/t6038-merge-text-auto.sh
> @@ -18,6 +18,7 @@ test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
>  
>  test_expect_success setup '
>   git config core.autocrlf false &&
> + git config core.eol crlf &&
>  
>   echo first line | append_cr >file &&
>   echo first line >control_file &&
> @@ -72,10 +73,8 @@ test_expect_success 'Merge after setting text=auto' '
>   same line
>   EOF
>  
> - if test_have_prereq NATIVE_CRLF; then
> - append_cr expected.temp &&
> - mv expected.temp expected
> - fi &&
> + append_cr expected.temp &&
> + mv expected.temp expected &&
>   git config merge.renormalize true &&
>   git rm -fr . &&
>   rm -f .gitattributes &&
> @@ -90,10 +89,8 @@ test_expect_success 'Merge addition of text=auto' '
>   same line
>   EOF
>  
> - if test_have_prereq NATIVE_CRLF; then
> - append_cr expected.temp &&
> - mv expected.temp expected
> - fi &&
> + append_cr expected.temp &&
> + mv expected.temp expected &&
>   git config merge.renormalize true &&
>   git rm -fr . &&
>   rm -f .gitattributes &&
> @@ -104,15 +101,9 @@ test_expect_success 'Merge addition of text=auto' '
>  
>  test_expect_success 'Detect CRLF/LF conflict after setting text=auto' '
>   echo "<<<" >expected &&
> - if test_have_prereq NATIVE_CRLF; then
> - echo first line | append_cr >>expected &&
> - echo same line | append_cr >>expected &&
> - echo === | append_cr >>expected
> - else
> - echo first line >>expected &&
> - echo same line >>expected &&
> - echo === >>expected
> - fi &&
> + echo first line | append_cr >>expected &&
> + echo same line | append_cr >>expected &&
> + echo === | append_cr >>expected &&
>   echo first line | append_cr >>expected &&
>   echo same line | append_cr >>expected &&
>   echo ">>>" >>expected &&
> @@ -128,15 +119,9 @@ test_expect_success 'Detect LF/CRLF conflict from 
> addition of text=auto' '
>   echo "<<<" >expected &&
>   echo first line | append_cr >>expected &&
>   echo same line | append_cr >>expected &&
> - if test_have_prereq NATIVE_CRLF; then
> - echo === | append_cr >>expected &&
> - echo first line | append_cr >>expected &&
> - echo same line | append_cr >>expected
> - else
> - echo === >>expected &&
> - echo first line >>expected &&
> - echo same line >>expected
> - fi &&
> + echo === | append_cr >>expected &&
> + echo first line | append_cr >>expected &&
> + echo same line | append_cr >>expected &&
>   echo ">>>" >>expected &&
>   git config merge.renormalize false &&
>   rm -f .gitattributes &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller  writes:

> I'll just drop support for values
> in the first series.

I do not think an exact string match to support :(attr:eol=crlf) is
so bad.  The "crazy stuff" aka over-engineering is when it goes
beyond that, e.g. 'eol is set to one of these values"
--
To unsubscribe from this list: send the line "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_INDEX_FILE relative path breaks in subdir

[Joey Hess]

Junio C Hamano wrote:
> Joey Hess  writes:
> 
> > Appears to be a bug in git. Seems that it's assuming GIT_INDEX_FILE is
> > relative to the top of the worktree and not to the CWD.
> 
> I think that has always been the case.  You can always specify it as
> relative to the top.  Of course, you can use absolute.

I think it *has* always been that way, but is there anything in the
documentation that indicates this is the case? This could well be best
fixed in the documentation.

Hmm, at least GIT_OBJECT_DIRECTORY also behaves this way.
(OTOH, GIT_DIR does not behave this way).

-- 
see shy jo


signature.asc
Description: PGP signature

Re: [RFD PATCH 0/3] Free all the memory!

[Stefan Beller]

On Tue, May 17, 2016 at 11:16 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So as a developer I wish we would close all leaks that are non-concerning.
>
> Valgrind suppression (and if you use other tools, suppression for
> them) sounds like the way to go, I would think.
>
> Reducing false positive is a good goal; it helps to highlight the
> real problems.  But we need to find a way to do so without hurting
> the use by the end users by making them pay the unnecessary cost to
> free() at the end and by cluttering the code with #ifdefs that makes
> it easier to introduce subtle bugs.

That's why I think the `optional_free` is a good thing as it doesn't clutter
the code?

>
>> David writes:
>>> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse 
>>> patch.
>>>
>>> I don't think we care that much about "still reachable" memory -- I only 
>>> care about lost memory.  I could imagine, I guess, something that happens 
>>> to save a pointer to a bunch of memory that should be freed, but I don't 
>>> think that's the common case.
>>
>> As said above I'd want them to be fixed for me as a developer for
>> better automated tooling and detection. (The alternative to fix the automated
>> tooling is a no-no for me ;)
>
> Does the word "no-no" mean what you seem to think it means?  It
> sounds as if you are saying "fixing tools to reduce false positives
> is fundamentally wrong, I refuse to go in that direction".
>

I just mean, that I have not enough time to do that, so I won't.
I know however how to send patches to this list.
--
To unsubscribe from this list: send the line "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: [RFD PATCH 0/3] Free all the memory!

[Junio C Hamano]

Stefan Beller  writes:

> So as a developer I wish we would close all leaks that are non-concerning.

Valgrind suppression (and if you use other tools, suppression for
them) sounds like the way to go, I would think.

Reducing false positive is a good goal; it helps to highlight the
real problems.  But we need to find a way to do so without hurting
the use by the end users by making them pay the unnecessary cost to
free() at the end and by cluttering the code with #ifdefs that makes
it easier to introduce subtle bugs.

> David writes:
>> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse 
>> patch.
>>
>> I don't think we care that much about "still reachable" memory -- I only 
>> care about lost memory.  I could imagine, I guess, something that happens to 
>> save a pointer to a bunch of memory that should be freed, but I don't think 
>> that's the common case.
>
> As said above I'd want them to be fixed for me as a developer for
> better automated tooling and detection. (The alternative to fix the automated
> tooling is a no-no for me ;)

Does the word "no-no" mean what you seem to think it means?  It
sounds as if you are saying "fixing tools to reduce false positives
is fundamentally wrong, I refuse to go in that direction".

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

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Tue, May 17, 2016 at 11:05 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> I am not talking about crazy stuff here, but consider our own
>> .gitattributes file:
>>
>> * whitespace=!indent,trail,space
>> *.[ch] whitespace=indent,trail,space
>> *.sh whitespace=indent,trail,space
>>
>> Now I want to search for
>>
>> "the whitespace attribute that is set to at least trail and space"
>
> With :(attr:VAR=VAL) syntax, you can only look for whitespace
> attribute exactly set to "!indent,trail,space", so you can't.  So
> you are talking about crazy stuff after all, aren't you?

I think that stuck mentally with me from the "label" series,
as then you had to have an exact specification of the value list.

We don't do that any more, so in a very first series I can omit the
value parsing at all.

> Are you
> now extending it to do "whitespace~=indent" or something like that?
>
> You do not even need VAR=VAL form for your main topic.  You only
> need group-doc group-code etc. to look for "set to TRUE", no?
>
> I do not want to see us wasting too much time over-engineering it
> without having a concrete and useful use case, and "find path to
> whom whitespace checks are set for 'trail' and 'space'" is not.
> These comma-separated tokens augment WS_DEFAULT_RULE which has
> 'trail' already, so you do not look for 'trail' in the first place.
>
> "I want submodules under subs/ that is in either group-doc or
> group-code" is more reasonable and realistic use case, but that does
> not need any crazy stuff.  Either two separate pathspec elements,
>
> ":(attr:group-doc)subs/" ":(attr:group-code)/subs/"
>
> or if we are to do the "ORed collection of ANDed attrs", it would be
> something like:
>
> ":(attr:group-doc):(attr:group-code)/subs/"
>
> no?

Thanks for bringing me back to realities. I'll just drop support for values
in the first series.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] Ignore dirty submodule states during stash

[Vasily Titskiy]

As stash does not know how to deal with submodules,
it should not try to save/restore their states
as it leads to redundant merge conflicts.

Added test checks if 'stash pop' does not trigger merge conflicts
in submodules.

Signed-off-by: Vasily Titskiy 
---
 git-stash.sh |  2 +-
 t/t3903-stash.sh | 33 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..b500c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -116,7 +116,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
-   git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+   git diff --name-only --ignore-submodules -z HEAD -- 
>"$TMP-stagenames" &&
git update-index -z --add --remove --stdin 
<"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..dfe39a4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -731,4 +731,37 @@ test_expect_success 'stash list --cc shows combined diff' '
test_cmp expect actual
 '
 
+test_expect_success 'stash ignores changes in submodules' '
+   git init sub1 &&
+   (
+   cd sub1 &&
+   echo "x" >file1 &&
+   git add file1 &&
+   git commit -a -m "initial sub1"
+   ) &&
+   git submodule add ./. sub1 &&
+   echo "main" >file1 &&
+   git add file1 &&
+   git commit -a -m "initial main" &&
+   # make changes in submodule
+   (
+   cd sub1 &&
+   echo "y" >>file1 &&
+   git commit -a -m "change y"
+   ) &&
+   git commit sub1 -m "update reference" &&
+   # switch submodule to another revision
+   (
+   cd sub1 &&
+   echo "z" >>file1 &&
+   git commit -a -m "change z"
+   ) &&
+   # everything is prepared, check if changes in submodules are ignored
+   echo "local change" >>file1 &&
+   git stash save &&
+   git checkout HEAD~1 &&
+   git submodule update &&
+   git stash pop
+'
+
 test_done
-- 
2.1.4

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

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller  writes:

> I am not talking about crazy stuff here, but consider our own
> .gitattributes file:
>
> * whitespace=!indent,trail,space
> *.[ch] whitespace=indent,trail,space
> *.sh whitespace=indent,trail,space
>
> Now I want to search for
>
> "the whitespace attribute that is set to at least trail and space"

With :(attr:VAR=VAL) syntax, you can only look for whitespace
attribute exactly set to "!indent,trail,space", so you can't.  So
you are talking about crazy stuff after all, aren't you?  Are you
now extending it to do "whitespace~=indent" or something like that?

You do not even need VAR=VAL form for your main topic.  You only
need group-doc group-code etc. to look for "set to TRUE", no?

I do not want to see us wasting too much time over-engineering it
without having a concrete and useful use case, and "find path to
whom whitespace checks are set for 'trail' and 'space'" is not.
These comma-separated tokens augment WS_DEFAULT_RULE which has
'trail' already, so you do not look for 'trail' in the first place.

"I want submodules under subs/ that is in either group-doc or
group-code" is more reasonable and realistic use case, but that does
not need any crazy stuff.  Either two separate pathspec elements,

":(attr:group-doc)subs/" ":(attr:group-code)/subs/"

or if we are to do the "ORed collection of ANDed attrs", it would be
something like:

":(attr:group-doc):(attr:group-code)/subs/"

no?
--
To unsubscribe from this list: send the line "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: [RFD PATCH 0/3] Free all the memory!

[Stefan Beller]

On Mon, May 16, 2016 at 8:41 PM, Eric Sunshine  wrote:
> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller  wrote:
>> When using automated tools to find memory leaks, it is hard to distinguish
>> between actual leaks and intentional non-cleanups at the end of the program,
>> such that the actual leaks hide in the noise.
>>
>> The end goal of this (unfinished) series is to close all intentional memory
>> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
>> demonstrating how the beginning of such a series could look like.
>
> Considering the signal-to-noise ratio mentioned above, the goal seems
> reasonable, but why pollute the code with #ifdef's all over the place
> by making the cleanup conditional? If you're going though the effort
> of plugging all these leaks, it probably makes sense to do them
> unconditionally.

I tried that once upon a time. The resentment from the list was:

We're exiting soon anyway (e.g. some cmd_foo function). Letting the
operating system clean up after us is faster than when we do it, so don't
do it.

However it would be nice to have a distinction between "I know we're leaking
this memory, but we don't care for $REASONS" and a genuine leak that
should concern the developers.

So as a developer I wish we would close all leaks that are non-concerning.
As a user I don't care about those leaks.

David writes:
> AFAIK, nothing in the "definitely lost" category is fixed by your rev-parse 
> patch.
>
> I don't think we care that much about "still reachable" memory -- I only care 
> about lost memory.  I could imagine, I guess, something that happens to save 
> a pointer to a bunch of memory that should be freed, but I don't think that's 
> the common case.

As said above I'd want them to be fixed for me as a developer for
better automated tooling and detection. (The alternative to fix the automated
tooling is a no-no for me ;)

Matthieu Moy writes:
> One potential issue with this is: if all developers and the testsuite
> use this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be
> well tested, and still this is the one used by real people. For example,
> if there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we
> may not notice it.
>
> Perhaps it'd be better to activate FREE_ALL_MEMORY only when tools like
> valgrind or so is used.

That's a really good point. I'l keep it in mind for a reroll.


Eric Wong writes:
> I haven't checked for git, but I suspect we get speedups by not
> calling free().  I've never needed to profile git, but free() at
> exit has been a measurable bottleneck in other projects I've
> worked on.  Often, free() was more expensive than *alloc().

Thanks for reiterating that original response I got back then :)

>
> In any case, I like constant conditionals in C or inline wrappers
> macros over CPP #ifdefs littered inside functions:
>
> /* in git-compat-util.h */
> #ifdef FREE_ALL_MEMORY
> static inline void optional_free(void *ptr) {}
> #else
> #  define FREE_ALL_MEMORY (0)
> #  define optional_free(ptr) free(ptr)
> #endif
>
> /* inside any function: */
>if (FREE_ALL_MEMORY)
>big_function_which_calls_multiple_frees();
>

Shouldn't that be "#ifndef" instead? I really like the
"optional_free", I'll keep it in mind!

>
> Also Valgrind has suppression files, so code modifications may
> not be necessary at all.

But there are more tools than just valgrind. (Although valgrind is really good!)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 00/17] Port branch.c to use ref-filter's printing options

[Junio C Hamano]

Karthik Nayak  writes:

> Hello, sorry for the confusion, it's built on top of 'next' which contains
> f307218 (t6302: simplify non-gpg cases). The merge conflict is due to the
> commit made by you 1cca17df (Documentation: fix linkgit references).

That is not "confusion", but an "incorrect piece of information".

The series does not seem to apply on 'next', either.

Where did you exactly rebase on top of?  It is not on f307218, it is
not on 'next', 'next@{1}',... 'next@{8}'.

f3072180 (t6302: simplify non-gpg cases, 2016-05-09) was merged to
'next' at 9fcb98b2 (Merge branch 'es/test-gpg-tags' into next,
2016-05-10), but the series does not seem to apply there, either.

$ git co 9fcb98b2
Applying: ref-filter: implement %(if), %(then), and %(else) atoms
error: patch failed: Documentation/git-for-each-ref.txt:181
error: Documentation/git-for-each-ref.txt: patch does not apply
Patch failed at 0001 ref-filter: implement %(if), %(then), and %(else) atoms
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Not that a series built on top of any 'next' is directly usable.
You are forcing me to identify which topics in 'next' you depend on,
and build a topic that does not contain anything unrelated that is
in 'next' before starting to apply these patches.  Can you pick a
more appropriate place to base these patches on, please?  Why isn't
this based on 'master', for example?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Tue, May 17, 2016 at 10:34 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> Then while parsing ":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/",
>>
>> This syntax is not pleasant to parse IMHO as it is not clear if the token
>> after white space (-VAR2 here) is another attribute or the next part of
>> the list of VAR1, ...
>
> Remove the ambiguity by declaring that the list is always whitespace
> separated.  No whitespace in var, no whitespace in val, no quoting.
>
> The set of attributes with values expected to be used in the
> pathspec "attribute match" magic, I do not think there is anything
> that wants such a random arbitrary string.  The value side of an
> attribute with value, e.g. "eol=crlf", "conflict-marker-size=7", is
> designed to be a token that our C code is prepared to parse.

I am not talking about crazy stuff here, but consider our own
.gitattributes file:

* whitespace=!indent,trail,space
*.[ch] whitespace=indent,trail,space
*.sh whitespace=indent,trail,space

Now I want to search for

"the whitespace attribute that is set to at least trail and space"

We cannot use commas for the specification as they are used in .gitattributes,
because that would make it even harder, so
I would imagine this could be used:

 :(attr:whitespace=space trail)

See the whitespace separates the values, not the next variable.

To add another variable, I would suggest using a ':', such as

 :(attr:whitespace=space trail:text)

might a viable thing.



>
> In other words, if you match the parsing semantics of parse_attr()
> in attr.c, you are OK.  The attribute subsystem will not give users
> anything that is more complex than what that routine is prepared to
> parse, and that is a "whitespace separated list, no whitespace in
> attribute names, no whitespace in values, no quoting".
>
--
To unsubscribe from this list: send the line "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_INDEX_FILE relative path breaks in subdir

[Junio C Hamano]

Joey Hess  writes:

> Appears to be a bug in git. Seems that it's assuming GIT_INDEX_FILE is
> relative to the top of the worktree and not to the CWD.

I think that has always been the case.  You can always specify it as
relative to the top.  Of course, you can use absolute.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Junio C Hamano]

Stefan Beller  writes:

>> Then while parsing ":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/",
>
> This syntax is not pleasant to parse IMHO as it is not clear if the token
> after white space (-VAR2 here) is another attribute or the next part of
> the list of VAR1, ...

Remove the ambiguity by declaring that the list is always whitespace
separated.  No whitespace in var, no whitespace in val, no quoting.

The set of attributes with values expected to be used in the
pathspec "attribute match" magic, I do not think there is anything
that wants such a random arbitrary string.  The value side of an
attribute with value, e.g. "eol=crlf", "conflict-marker-size=7", is
designed to be a token that our C code is prepared to parse.

In other words, if you match the parsing semantics of parse_attr()
in attr.c, you are OK.  The attribute subsystem will not give users
anything that is more complex than what that routine is prepared to
parse, and that is a "whitespace separated list, no whitespace in
attribute names, no whitespace in values, no quoting".

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

Re: [Bug] git-log prints wrong unixtime with --date=format:%s

[Michael Heerdegen]

Michael Heerdegen  writes:

> the command
>
>git log --pretty=format:%ad --date=format:%s
>
> displays wrong unixtime values; apparently how much the printed value
> differs from the expected value depends on the system's time zone and
> whether daylight savings time is enabled or not.

Two more comments:

  - --date=raw seems to work correctly (though the doc tells it would
also use "%s").

  - When specifying versions like @{N hours}, the result seems to differ
(by one hour?) from what I expect, too.


Thanks,

Michael.

--
To unsubscribe from this list: send the line "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_INDEX_FILE relative path breaks in subdir

[Joey Hess]

joey@darkstar:/tmp>git init test
Initialized empty Git repository in /tmp/test/.git/
joey@darkstar:/tmp>cd test
joey@darkstar:/tmp/test>mkdir sub
joey@darkstar:/tmp/test>cd sub
joey@darkstar:/tmp/test/sub>GIT_INDEX_FILE=../.git/otherindex git write-tree
fatal: Unable to create '/tmp/test/../.git/otherindex.lock': No such file or 
directory

Appears to be a bug in git. Seems that it's assuming GIT_INDEX_FILE is
relative to the top of the worktree and not to the CWD.

Workaround: Use absolute path to the index file.

joey@darkstar:/tmp/test/sub>GIT_INDEX_FILE=`pwd`/../.git/otherindex git 
write-tree
4b825dc642cb6eb9a060e54bf8d69288fbee4904
joey@darkstar:/tmp/test/sub>ls ../.git/otherindex 
../.git/otherindex

git version 2.8.1

-- 
see shy jo


signature.asc
Description: PGP signature

Re: [PATCH v2] Ignore dirty submodule states during stash

[Vasily Titskiy]

The command does nothing, so it's not needed. There is also a typo in
my patch description, so I'll resend it again with needed changes.
--
  Regards,
  Vasily Titskiy


On Tue, May 17, 2016 at 1:04 PM, Junio C Hamano  wrote:
> Vasily Titskiy  writes:
>
>> You're right, it's redundant here. Should I resubmit the path without this 
>> line?
>
> I wasn't pointing out that it was not needed.  I was only asking
> what it was meant to do.
>
> If you now think it shouldn't have been there, that merely means
> your code was wrong.  It does not mean I'm right ;-)
>
> With that line removed, would the patch now becomes right?  Are
> there other bugs?
--
To unsubscribe from this list: send the line "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] Ignore dirty submodule states during stash

[Junio C Hamano]

Vasily Titskiy  writes:

> You're right, it's redundant here. Should I resubmit the path without this 
> line?

I wasn't pointing out that it was not needed.  I was only asking
what it was meant to do.

If you now think it shouldn't have been there, that merely means
your code was wrong.  It does not mean I'm right ;-)

With that line removed, would the patch now becomes right?  Are
there other bugs?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Mon, May 16, 2016 at 10:03 PM, Junio C Hamano  wrote:
>
> int attr_match_nr;
> int attr_match_alloc;
> struct attr_match {
> struct git_attr *attr;
> char *value;
> enum attr_match_mode {
> ...
> } match_mode;
> } *attr_match;
> struct git_attr_check *attr_check;

ok, I'll use that structure and go from here.


>
> Then while parsing ":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/",

This syntax is not pleasant to parse IMHO as it is not clear if the token
after white space (-VAR2 here) is another attribute or the next part of
the list of VAR1, i.e. this could also be:

:(attr:VAR1=VAL1 VAL2)

I wonder if we'd want to use the colon to separate different VARs:

:(attr:VAR1=VAL1 VAL2:-VAR2:VAR3)

which means:

   match all path, that
* have VAR1 set to a value list containing at least
  VAL1 and VAL2
* have VAR2 unset
* have VAR3 set to 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 10/12] attr: convert git_all_attrs() to use "struct git_attr_check"

[Junio C Hamano]

Junio C Hamano  writes:

> +struct git_attr_check *git_attr_check_alloc(void)
> +{
> + return xcalloc(1, sizeof(struct git_attr_check));
> +}
> +
> +void git_attr_check_append(struct git_attr_check *check, const char *name)
> +{
> + struct git_attr *attr;
> +
> + if (check->finalized)
> + die("BUG: append after git_attr_check structure is finalized");
> +
> + attr = git_attr(name);
> + if (!attr)
> + die("%s: not a valid attribute name", name);
> + ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
> + check->check[check->check_nr++].attr = attr;
> +}

Given that one of the two expected callers, namely, "check-attr" and
Stefan's pathspec label magic, of this "alloc and then append" side
of the API wants to have an access to git_attr(name), I think
the function signature for this one should be updated to take not
"const char *name" but instead take "struct git_attr *attr", i.e.

void git_attr_check_append(struct git_attr_check *check,
   struct git_attr *attr);

Then the loop in check-attr below

> + check = git_attr_check_alloc();
> + for (i = 0; i < cnt; i++)
> + git_attr_check_append(check, argv[i]);

would become

check = git_attr_check_alloc();
for (i = 0; i < cnt; i++)
git_attr_check_append(check, git_attr(argv[i]));

And this part in $gmane/294855

Then while parsing ":(attr:VAR1=VAL1 -VAR2 VAR3...)path/to/dir/",
you would first do:

p->attr_check = git_attr_check_alloc();

once, and then send each of VAR1=VAL2, -VAR2, VAR3... to your
parse_one_item() helper function which would:

 * parse the match-mode like your code does;

 * parse out the attribute name (i.e. VAR1, VAR2 and VAR3), and
   instead of keeping it as a "(const) char *", call git_attr()
   to intern it (and keep it in local variable "attr"), and save
   it in p->attr_match[p->attr_nr].attr;

 * call git_attr_check_append(p->attr_check, git_attr_name(attr))

would look like

... saw ":(attr:", expect "VAR1=VAL1 -VAR2...)" to follow;
... char *scan points at byte after attr: now.

item->attr_check = git_attr_check_alloc();
while (scan) {
const char *var, *val;
enum attr_match_mode mode;
struct git_attr *attr;
struct attr_match *match;

scan = parse_attr_match(scan, , , );
... var points at VAR1, val points at VAL,
... mode becomes MATCH_VALUE; scan skips
... forward to pint at -VAR2.  parse_attr_match()
... would return NULL once input runs out.

ALLOC_GROW(item->attr_match, ...);
match = >attr_match[item->attr_match_nr++];

attr = git_attr(var);

match->attr = attr;
match->value = val;
match->match_mode = mode;

git_attr_check_append(item->attr_check, attr);
}

Then matching part would get the "item" and would do:

... after making sure that the path matches
... the pathspec the magic is attached to ...

git_check_attr(path, item->attr_check);
for (i = 0; i < item->attr_match_nr; i++) {
struct git_attr *attr = item->attr_check->check[i].attr;
const char *value = item->attr_check->check[i].value;
struct attr_match *match = >attr_match[i];

... compare what "match" expects with  ...
}

--
To unsubscribe from this list: send the line "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] Ignore dirty submodule states during stash

[Eric Sunshine]

On Tue, May 17, 2016 at 9:16 AM, Vasily Titskiy  wrote:
> As stash does not know how to deal with submodules,
> it should not try to save/restore their states
> as it leads to redundant merge conflicts.
>
> Added test checks if 'stash pop' does not trigger merge conflics

/conflics/conflicts/

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

Re: [RFC-PATCHv6 4/4] pathspec: allow querying for attributes

[Stefan Beller]

On Mon, May 16, 2016 at 9:23 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> + * attr:+val to find value set to true
>> + * attr:-val to find a value set to false
>> + * attr:!val to find a value that is not set
>> + * (i.e. it is neither set as "val", "val=", nor unset as "-val")
>> + * attr:val=value: to find value that have at least a and b set.
>
> I would have expected that there won't be "attr:+val", but it is
> spelled as "attr:val" instead.

"val" matches if the attr is not unspecified, i.e. one of {true, false, value}
"+val" matches {true} only.

Maybe we want to redo that to

"val" matches {true} only.
"?val" matches {true, false, value}. (I can leave this case out in the
first series, too)

>
>> +static void parse_attr_item(struct attr_item *attr, const char *value)
>
> Please do not call something that is not part of the attribute
> infrastructure as "attr_item"; I wasted time looking for the
> structure definition for "attr_item" in .

So "parse_pathspec_attr_match" instead?

>
>> +static int match_attrs(const char *name, int namelen,
>> +const struct pathspec_item *item)
>> +{
>> + char *path;
>> + int i;
>> +
>> + if (!check) {
>> + check = git_attr_check_alloc();
>> + for (i = 0; i < item->attr_nr; i++)
>> + git_attr_check_append(check, item->attrs[i].attr);
>> + }
>> +
>> + path = xmemdupz(name, namelen);
>> + git_all_attrs(path, check);
>
> PLEASE DON'T.  git_all_attrs() asks for all the attribute under the
> sun and has no hope to perform sensibly, especially at the very leaf
> level of the pathspec logic where one call to this function is made
> for each and every path in the tree.

This is executed only once, as check is static? From a users perception
it doesn't matter if it is executed once just after parsing all pathspec
items or at the first path to check for a match, no?

The mistake is using the API wrong. So inside the '!check', after the
preparation loop of git_attr_check_append, we'd need to
hand over the "check" to git_check_attr instead?

>
> Instead, have a pointer to "struct git_attr_check" in pathspec_item
> and make a call to git_check_attr(path, item->check) here.

I see, then we have multiple `check` structs. Makes sense.

>
> Which means that you would need to prepare git_attr_check around ...
>
>> + if (skip_prefix(copyfrom, "attr:", )) {
>> + ALLOC_GROW(item->attrs, item->attr_nr + 1, 
>> item->attr_alloc);
>> + parse_attr_item(>attrs[item->attr_nr++], body);
>
> ... HERE.
>
--
To unsubscribe from this list: send the line "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] Ignore dirty submodule states during stash

[Vasily Titskiy]

Hi Junio,

You're right, it's redundant here. Should I resubmit the path without this line?

--
  Regards,
  Vasily Titskiy

On Tue, May 17, 2016 at 12:15 PM, Junio C Hamano  wrote:
>
> Vasily Titskiy  writes:
>
> > diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> > index 2142c1f..1be62f3 100755
> > --- a/t/t3903-stash.sh
> > +++ b/t/t3903-stash.sh
> > @@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined 
> > diff' '
> >   test_cmp expect actual
> >  '
> >
> > +test_expect_success 'stash ignores changes in submodules' '
> > + git submodule init &&
>
> H... what is this "submodule init" needed for at this point in
> the test sequence?
>
> > + git init sub1 &&
> > + (
> > + cd sub1 &&
> > + echo "x" >file1 &&
> > + git add file1 &&
> > + git commit -a -m "initial sub1"
> > + ) &&
> > + git submodule add ./. sub1 &&
> > + echo "main" >file1 &&
> > + git add file1 &&
> > + git commit -a -m "initial main" &&
> > + # make changes in submodule
> > + (
> > + cd sub1 &&
> > + echo "y" >>file1 &&
> > + git commit -a -m "change y"
> > + ) &&
> > + git commit sub1 -m "update reference" &&
> > + # switch submodule to another revision
> > + (
> > + cd sub1 &&
> > + echo "z" >>file1 &&
> > + git commit -a -m "change z"
> > + ) &&
> > + # everything is prepared, check if changes in submodules are ignored
> > + echo "local change" >>file1 &&
> > + git stash save &&
> > + git checkout HEAD~1 &&
> > + git submodule update &&
> > + git stash pop
> > +'
> > +
> >  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/2] convert: ce_compare_data() checks for a sha1 of a path

[tboegi]

From: Torsten Bögershausen 

To compare a file in working tree with the index, convert_to_git() is used,
the the result is hashed and the hash value compared with ce->sha1.

Deep down would_convert_crlf_at_commit() is invoked, to check if CRLF
are converted or not: When a CRLF had been in the index before, CRLF in
the working tree are not converted.

While in a merge, a file name in the working tree has different blobs
in the index with different hash values.
Forwarding ce->sha1 from ce_compare_data() into crlf_to_git() makes sure
the would_convert_crlf_at_commit() looks at the appropriate blob.

has_cr_in_index() does not use read_blob_data_from_cache() any more.
A single function to read data for a give sha value makes it easier to
refactor has_cr_in_index() to use the streaming interface.

Signed-off-by: Torsten Bögershausen 
---
 cache.h  |  1 +
 convert.c| 34 ++
 convert.h| 23 +++
 read-cache.c |  4 +++-
 sha1_file.c  | 17 +
 5 files changed, 58 insertions(+), 21 deletions(-)

diff --git a/cache.h b/cache.h
index 15a2a10..a5136c0 100644
--- a/cache.h
+++ b/cache.h
@@ -605,6 +605,7 @@ extern int ie_modified(const struct index_state *, const 
struct cache_entry *, s
 
 #define HASH_WRITE_OBJECT 1
 #define HASH_FORMAT_CHECK 2
+#define HASH_CE_HAS_SHA1  4
 extern int index_fd(unsigned char *sha1, int fd, struct stat *st, enum 
object_type type, const char *path, unsigned flags);
 extern int index_path(unsigned char *sha1, const char *path, struct stat *st, 
unsigned flags);
 
diff --git a/convert.c b/convert.c
index f524b8d..c972d14 100644
--- a/convert.c
+++ b/convert.c
@@ -217,21 +217,28 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
}
 }
 
-static int has_cr_in_index(const char *path)
+static int has_cr_in_index(const char *path, const unsigned char *sha1)
 {
unsigned long sz;
void *data;
-   int has_cr;
-
-   data = read_blob_data_from_cache(path, );
+   int has_cr = 0;
+   enum object_type type;
+   if (!sha1)
+   sha1 = get_sha1_from_cache(path);
+   if (!sha1)
+   return 0;
+   data = read_sha1_file(sha1, , );
if (!data)
return 0;
-   has_cr = memchr(data, '\r', sz) != NULL;
+   if (type == OBJ_BLOB)
+   has_cr = memchr(data, '\r', sz) != NULL;
+
free(data);
return has_cr;
 }
 
-static int crlf_to_git(const char *path, const char *src, size_t len,
+static int crlf_to_git(const char *path, const unsigned char *sha1,
+  const char *src, size_t len,
   struct strbuf *buf,
   enum crlf_action crlf_action, enum safe_crlf checksafe)
 {
@@ -260,7 +267,7 @@ 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, sha1))
return 0;
}
}
@@ -852,8 +859,9 @@ const char *get_convert_attr_ascii(const char *path)
return "";
 }
 
-int convert_to_git(const char *path, const char *src, size_t len,
-   struct strbuf *dst, enum safe_crlf checksafe)
+int convert_to_git_ce_sha1(const char *path, const unsigned char *sha1,
+  const char *src, size_t len,
+  struct strbuf *dst, enum safe_crlf checksafe)
 {
int ret = 0;
const char *filter = NULL;
@@ -874,7 +882,7 @@ int convert_to_git(const char *path, const char *src, 
size_t len,
src = dst->buf;
len = dst->len;
}
-   ret |= crlf_to_git(path, src, len, dst, ca.crlf_action, checksafe);
+   ret |= crlf_to_git(path, sha1, src, len, dst, ca.crlf_action, 
checksafe);
if (ret && dst) {
src = dst->buf;
len = dst->len;
@@ -882,7 +890,9 @@ int convert_to_git(const char *path, const char *src, 
size_t len,
return ret | ident_to_git(path, src, len, dst, ca.ident);
 }
 
-void convert_to_git_filter_fd(const char *path, int fd, struct strbuf *dst,
+void convert_to_git_filter_fd(const char *path,
+ const unsigned char *sha1,
+ int fd, struct strbuf *dst,
  enum safe_crlf checksafe)
 {
struct conv_attrs ca;
@@ -894,7 +904,7 @@ void convert_to_git_filter_fd(const char *path, int fd, 
struct strbuf *dst,
if (!apply_filter(path, NULL, 0, fd, dst, ca.drv->clean))
die("%s: clean filter '%s' failed", path, ca.drv->name);
 
-   crlf_to_git(path, dst->buf, dst->len, dst, ca.crlf_action, checksafe);
+   

[PATCH v4 1/2] read-cache: factor out get_sha1_from_index() helper

[tboegi]

From: Torsten Bögershausen 

Factor out the retrieval of the sha1 for a given path in
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 
---
 cache.h  |  3 +++
 read-cache.c | 29 ++---
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 160f8e3..15a2a10 100644
--- a/cache.h
+++ b/cache.h
@@ -379,6 +379,7 @@ extern void free_name_hash(struct index_state *istate);
 #define unmerge_cache_entry_at(at) unmerge_index_entry_at(_index, at)
 #define unmerge_cache(pathspec) unmerge_index(_index, pathspec)
 #define read_blob_data_from_cache(path, sz) 
read_blob_data_from_index(_index, (path), (sz))
+#define get_sha1_from_cache(path)  get_sha1_from_index (_index, (path))
 #endif
 
 enum object_type {
@@ -1038,6 +1039,8 @@ static inline void *read_sha1_file(const unsigned char 
*sha1, enum object_type *
return read_sha1_file_extended(sha1, type, size, LOOKUP_REPLACE_OBJECT);
 }
 
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path);
+
 /*
  * This internal function is only declared here for the benefit of
  * lookup_replace_object().  Please do not call it directly.
diff --git a/read-cache.c b/read-cache.c
index d9fb78b..a3ef967 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -2263,13 +2263,27 @@ int index_name_is_other(const struct index_state 
*istate, const char *name,
 
 void *read_blob_data_from_index(struct index_state *istate, const char *path, 
unsigned long *size)
 {
-   int pos, len;
+   const unsigned char *sha1;
unsigned long sz;
enum object_type type;
void *data;
 
-   len = strlen(path);
-   pos = index_name_pos(istate, path, len);
+   sha1 = get_sha1_from_index(istate, path);
+   if (!sha1)
+   return NULL;
+   data = read_sha1_file(sha1, , );
+   if (!data || type != OBJ_BLOB) {
+   free(data);
+   return NULL;
+   }
+   if (size)
+   *size = sz;
+   return data;
+}
+
+const unsigned char *get_sha1_from_index(struct index_state *istate, const 
char *path)
+{
+   int pos = index_name_pos(istate, path, strlen(path));
if (pos < 0) {
/*
 * We might be in the middle of a merge, in which
@@ -2285,14 +2299,7 @@ void *read_blob_data_from_index(struct index_state 
*istate, const char *path, un
}
if (pos < 0)
return NULL;
-   data = read_sha1_file(istate->cache[pos]->sha1, , );
-   if (!data || type != OBJ_BLOB) {
-   free(data);
-   return NULL;
-   }
-   if (size)
-   *size = sz;
-   return data;
+   return (istate->cache[pos]->sha1);
 }
 
 void stat_validity_clear(struct stat_validity *sv)
-- 
2.0.0.rc1.6318.g0c2c796

--
To unsubscribe from this list: send the line "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 0/2] CRLF: ce_compare_data() checks for a sha1 of a path

[tboegi]

From: Torsten Bögershausen 

Break up the old 10/10 series about CLRF handling into smaller
series. This is a small bugfix, when merge.renormalize is used
with core.autocrlf (and no attributes are set).
Prepare the refactoring to use the streaming interface.

Torsten Bögershausen (2):
  read-cache: factor out get_sha1_from_index() helper
  convert: ce_compare_data() checks for a sha1 of a path

 cache.h  |  4 
 convert.c| 34 ++
 convert.h| 23 +++
 read-cache.c | 33 +
 sha1_file.c  | 17 +
 5 files changed, 79 insertions(+), 32 deletions(-)

-- 
2.0.0.rc1.6318.g0c2c796

--
To unsubscribe from this list: send the line "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] Ignore dirty submodule states during stash

[Junio C Hamano]

Vasily Titskiy  writes:

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 2142c1f..1be62f3 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined 
> diff' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'stash ignores changes in submodules' '
> + git submodule init &&

H... what is this "submodule init" needed for at this point in
the test sequence?

> + git init sub1 &&
> + (
> + cd sub1 &&
> + echo "x" >file1 &&
> + git add file1 &&
> + git commit -a -m "initial sub1"
> + ) &&
> + git submodule add ./. sub1 &&
> + echo "main" >file1 &&
> + git add file1 &&
> + git commit -a -m "initial main" &&
> + # make changes in submodule
> + (
> + cd sub1 &&
> + echo "y" >>file1 &&
> + git commit -a -m "change y"
> + ) &&
> + git commit sub1 -m "update reference" &&
> + # switch submodule to another revision
> + (
> + cd sub1 &&
> + echo "z" >>file1 &&
> + git commit -a -m "change z"
> + ) &&
> + # everything is prepared, check if changes in submodules are ignored
> + echo "local change" >>file1 &&
> + git stash save &&
> + git checkout HEAD~1 &&
> + git submodule update &&
> + git stash pop
> +'
> +
>  test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/1] t6038; use crlf on all platforms

[tboegi]

From: Torsten Bögershausen 

t6038 uses different code, depending if NATIVE_CRLF is set ot not.
When the native line endings are LF, merge.renormalize is not tested very well.
Change the test to always use CRLF by setting core.eol=crlf.
---
Broke the 10/10 series into smaller pieces, this is the update of t6038
 t/t6038-merge-text-auto.sh | 37 +++--
 1 file changed, 11 insertions(+), 26 deletions(-)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 85c10b0..4dc8c1a 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh
@@ -18,6 +18,7 @@ test_have_prereq SED_STRIPS_CR && SED_OPTIONS=-b
 
 test_expect_success setup '
git config core.autocrlf false &&
+   git config core.eol crlf &&
 
echo first line | append_cr >file &&
echo first line >control_file &&
@@ -72,10 +73,8 @@ test_expect_success 'Merge after setting text=auto' '
same line
EOF
 
-   if test_have_prereq NATIVE_CRLF; then
-   append_cr expected.temp &&
-   mv expected.temp expected
-   fi &&
+   append_cr expected.temp &&
+   mv expected.temp expected &&
git config merge.renormalize true &&
git rm -fr . &&
rm -f .gitattributes &&
@@ -90,10 +89,8 @@ test_expect_success 'Merge addition of text=auto' '
same line
EOF
 
-   if test_have_prereq NATIVE_CRLF; then
-   append_cr expected.temp &&
-   mv expected.temp expected
-   fi &&
+   append_cr expected.temp &&
+   mv expected.temp expected &&
git config merge.renormalize true &&
git rm -fr . &&
rm -f .gitattributes &&
@@ -104,15 +101,9 @@ test_expect_success 'Merge addition of text=auto' '
 
 test_expect_success 'Detect CRLF/LF conflict after setting text=auto' '
echo "<<<" >expected &&
-   if test_have_prereq NATIVE_CRLF; then
-   echo first line | append_cr >>expected &&
-   echo same line | append_cr >>expected &&
-   echo === | append_cr >>expected
-   else
-   echo first line >>expected &&
-   echo same line >>expected &&
-   echo === >>expected
-   fi &&
+   echo first line | append_cr >>expected &&
+   echo same line | append_cr >>expected &&
+   echo === | append_cr >>expected &&
echo first line | append_cr >>expected &&
echo same line | append_cr >>expected &&
echo ">>>" >>expected &&
@@ -128,15 +119,9 @@ test_expect_success 'Detect LF/CRLF conflict from addition 
of text=auto' '
echo "<<<" >expected &&
echo first line | append_cr >>expected &&
echo same line | append_cr >>expected &&
-   if test_have_prereq NATIVE_CRLF; then
-   echo === | append_cr >>expected &&
-   echo first line | append_cr >>expected &&
-   echo same line | append_cr >>expected
-   else
-   echo === >>expected &&
-   echo first line >>expected &&
-   echo same line >>expected
-   fi &&
+   echo === | append_cr >>expected &&
+   echo first line | append_cr >>expected &&
+   echo same line | append_cr >>expected &&
echo ">>>" >>expected &&
git config merge.renormalize false &&
rm -f .gitattributes &&
-- 
2.0.0.rc1.6318.g0c2c796

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

[PATCH v2] Ignore dirty submodule states during stash

[Vasily Titskiy]

As stash does not know how to deal with submodules,
it should not try to save/restore their states
as it leads to redundant merge conflicts.

Added test checks if 'stash pop' does not trigger merge conflics
in submodules.

Signed-off-by: Vasily Titskiy 
---
 git-stash.sh |  2 +-
 t/t3903-stash.sh | 34 ++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/git-stash.sh b/git-stash.sh
index c7c65e2..b500c44 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -116,7 +116,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
-   git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+   git diff --name-only --ignore-submodules -z HEAD -- 
>"$TMP-stagenames" &&
git update-index -z --add --remove --stdin 
<"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 2142c1f..1be62f3 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -731,4 +731,38 @@ test_expect_success 'stash list --cc shows combined diff' '
test_cmp expect actual
 '
 
+test_expect_success 'stash ignores changes in submodules' '
+   git submodule init &&
+   git init sub1 &&
+   (
+   cd sub1 &&
+   echo "x" >file1 &&
+   git add file1 &&
+   git commit -a -m "initial sub1"
+   ) &&
+   git submodule add ./. sub1 &&
+   echo "main" >file1 &&
+   git add file1 &&
+   git commit -a -m "initial main" &&
+   # make changes in submodule
+   (
+   cd sub1 &&
+   echo "y" >>file1 &&
+   git commit -a -m "change y"
+   ) &&
+   git commit sub1 -m "update reference" &&
+   # switch submodule to another revision
+   (
+   cd sub1 &&
+   echo "z" >>file1 &&
+   git commit -a -m "change z"
+   ) &&
+   # everything is prepared, check if changes in submodules are ignored
+   echo "local change" >>file1 &&
+   git stash save &&
+   git checkout HEAD~1 &&
+   git submodule update &&
+   git stash pop
+'
+
 test_done
-- 
2.1.4

--
To unsubscribe from this list: send the line "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] t9801 and t9803 broken on next

[Jeff King]

On Tue, May 17, 2016 at 10:07:16AM +0200, Lars Schneider wrote:

> I think that is pretty much the problem. Here is what is happening:
> 
> 1.  git-p4 imports all changelists for the "main" branch
> 
> 2.  git-p4 starts to import a second branch and creates a fast-import
> "progress checkpoint". This triggers:
> 
> --> fast-import.c: cycle_packfile
> --> fast-import.c: end_packfile
> --> fast-import.c: loosen_small_pack
> 
> At this point we have no packfile anymore.
> 
> 3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
> to fast-import in order to create a new branch. This triggers:
> 
> --> fast-import.c: parse_new_commit
> --> fast-import.c: load_branch
> --> fast-import.c: load_tree
> 
> "load_tree" calls "find_object" and the result has a "pack_id" of 0.
> I think this indicates that the object is in the packfile. Shouldn't
> the "pack_id" be "MAX_PACK_ID" instead?
> 
> myoe = find_object(sha1);
> if (myoe && myoe->pack_id != MAX_PACK_ID) {

Thanks for the analysis. I think this is definitely the problem.  After
fast-import finalizes a packfile (either at the end of the program or
due to a checkpoint), it never discards its internal mapping of objects
to pack locations. It _has_ to keep such a mapping before the pack is
finalized, because git's regular object database doesn't know about it.
But afterwards, it should be able to rely on the object database.

Retaining the mapping at the end of the program is obviously OK because
we won't make any more lookups in it.

Retaining it at a checkpoint when we keep the packfile is OK, because we
can (usually) still access that packfile (the exception would be if
somebody runs "git repack" while fast-import is running).

But obviously a checkpoint where we throw away the pack needs to clear
that mapping.

The patch below probably makes your case work, but there are a lot of
open questions:

  1. Should we always discard the mapping, even when not loosening
 objects? I can't think of a real downside to always using git's
 object lookups.

  2. Can we free memory associated with object_entry structs at this
 point? They won't be accessible via the hash, but might other bits
 of the code have kept pointers to them?

 I suspect it may screw up the statistics that fast-import prints at
 the end, but that's a minor point.

  3. I notice that a few other structs (branch and tag) hold onto the
 pack_id, which will now point to a pack we can't access. Does this
 matter? I don't think so, because checkpoint() seems to dump the
 branches and tags.

  4. In general, should we be loosening objects at checkpoints at all?

 I guess it is probably more efficient than creating a bunch of
 little packs. And it should probably not happen much at all outside
 of tests (i.e., callers should generally checkpoint after an
 appreciable amount of work is done).

diff --git a/fast-import.c b/fast-import.c
index 0e8bc6a..9bfbfb0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -597,6 +597,22 @@ static struct object_entry *insert_object(unsigned char 
*sha1)
return e;
 }
 
+static void clear_object_table(void)
+{
+   unsigned int h;
+   for (h = 0; h < ARRAY_SIZE(object_table); h++) {
+   /*
+* We can't individually free objects here
+* because they are allocated from a pool.
+*/
+   object_table[h] = NULL;
+   }
+   /*
+* XXX maybe free object_entry_pool here,
+* or might something still be referencing them?
+*/
+}
+
 static unsigned int hc_str(const char *s, size_t len)
 {
unsigned int r = 0;
@@ -1035,6 +1051,9 @@ discard_pack:
pack_data = NULL;
running = 0;
 
+   /* The objects are now available via git's regular lookups. */
+   clear_object_table();
+
/* We can't carry a delta across packfiles. */
strbuf_release(_blob.data);
last_blob.offset = 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: [RFD PATCH 0/3] Free all the memory!

[Eric Wong]

Eric Sunshine  wrote:
> On Mon, May 16, 2016 at 11:22 PM, Stefan Beller  wrote:
> > When using automated tools to find memory leaks, it is hard to distinguish
> > between actual leaks and intentional non-cleanups at the end of the program,
> > such that the actual leaks hide in the noise.
> 
> Considering the signal-to-noise ratio mentioned above, the goal seems
> reasonable, but why pollute the code with #ifdef's all over the place
> by making the cleanup conditional? If you're going though the effort
> of plugging all these leaks, it probably makes sense to do them
> unconditionally.

I haven't checked for git, but I suspect we get speedups by not
calling free().  I've never needed to profile git, but free() at
exit has been a measurable bottleneck in other projects I've
worked on.  Often, free() was more expensive than *alloc().

In any case, I like constant conditionals in C or inline wrappers
macros over CPP #ifdefs littered inside functions:

/* in git-compat-util.h */
#ifdef FREE_ALL_MEMORY
static inline void optional_free(void *ptr) {}
#else
#  define FREE_ALL_MEMORY (0)
#  define optional_free(ptr) free(ptr)
#endif

/* inside any function: */
if (FREE_ALL_MEMORY)
big_function_which_calls_multiple_frees();


Also Valgrind has suppression files, so code modifications may
not be necessary at all.
--
To unsubscribe from this list: send the line "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] t9801 and t9803 broken on next

[Eric Wong]

Lars Schneider  wrote:
> I am no expert in "fast-import". Does anyone have a few hints how to
> proceed?

I don't know fast-import or the C internals of git well at all,
either.  But capturing the exact input to fast-import (using
tee) would be useful for non-p4 users to debug the problem
and would go a long way in reproducing a standalone test case.

I'm Python-illiterate , but hopefully this works
to capture the stdin fed to fast-import (totally untested):

--- a/git-p4.py
+++ b/git-p4.py
@@ -3366,7 +3366,8 @@ class P4Sync(Command, P4UserMap):
 
 self.tz = "%+03d%02d" % (- time.timezone / 3600, ((- time.timezone % 
3600) / 60))
 
-self.importProcess = subprocess.Popen(["git", "fast-import"],
+cmd = [ "sh", "-c", "tee /tmp/gfi-in.$$ | git fast-import" ]
+self.importProcess = subprocess.Popen(cmd,
   stdin=subprocess.PIPE,
   stdout=subprocess.PIPE,
   stderr=subprocess.PIPE);
--
To unsubscribe from this list: send the line "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: [RFD PATCH 0/3] Free all the memory!

[Matthieu Moy]

Stefan Beller  writes:

> The end goal of this (unfinished) series is to close all intentional memory
> leaks when enabling the -DFREE_ALL_MEMORY switch. This is just
> demonstrating how the beginning of such a series could look like.

One potential issue with this is: if all developers and the testsuite
use this -DFREE_ALL_MEMORY, the non-free-all-memory setup will not be
well tested, and still this is the one used by real people. For example,
if there's a really annoying memory leak hidden by FREE_ALL_MEMORY, we
may not notice it.

Perhaps it'd be better to activate FREE_ALL_MEMORY only when tools like
valgrind or so is used.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "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] t9801 and t9803 broken on next

[Luke Diamand]

On 14 May 2016 at 19:15, Junio C Hamano  wrote:
> Lars Schneider  writes:
>
>>> On 13 May 2016, at 18:37, Junio C Hamano  wrote:
>>>
>>> Are you saying that "git p4" itself breaks unless fast-import always
>>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>>> if the test script is somehow looking at the number of packs or
>>> loose objects and declaring a failure, even when the resulting
>>> history in p4 and git are correct, then that is a different issue,
>>> and forcing to explode a tiny pack is a reasonable workaround.  I
>>> couldn't quite tell which the case is.
>>>
>>> Puzzled.  Please help.
>>
>> t9801 "import depot, branch detection" is the first test that fails
>> with a fast import error:
>> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>>
>> fast-import crash report:
>> fast-import process: 77079
>> parent process : 77077
>> at 2016-05-14 07:48:40 +
>>
>> fatal: offset beyond end of packfile (truncated pack?)
>
> Hmm, does that suggest Eric's "explode them into loose instead of
> keeping a small pack" insufficient?  It sounds like that somebody
> wanted to read some data back from its packfile without knowing that
> the updated code may make it available in a loose object form, which
> would mean that somebody needs to be told about what is going on,
> namely, these objects are not in a half-written pack but are found
> as loose objects.
>
>> The problem seems to be related to the git-p4 branch import.
>> I don't have time this weekend to look further into it, but
>> I will try next week.

The last thing that fast-import is sent before it crashes is this:

checkpoint
  progress checkpoint
  commit refs/remotes/p4/depot/branch2
  mark :6
  committer Dr. author  1463212118 +
  data 

Re: [BUG] t9801 and t9803 broken on next

[Lars Schneider]

> On 14 May 2016, at 20:15, Junio C Hamano  wrote:
> 
> Lars Schneider  writes:
> 
>>> On 13 May 2016, at 18:37, Junio C Hamano  wrote:
>>> 
>>> Are you saying that "git p4" itself breaks unless fast-import always
>>> writes a new (tiny) packfile?  That sounds quite broken, and setting
>>> unpacklimit to 0 does not sound like a sensible "fix".  Of course,
>>> if the test script is somehow looking at the number of packs or
>>> loose objects and declaring a failure, even when the resulting
>>> history in p4 and git are correct, then that is a different issue,
>>> and forcing to explode a tiny pack is a reasonable workaround.  I
>>> couldn't quite tell which the case is.
>>> 
>>> Puzzled.  Please help.
>> 
>> t9801 "import depot, branch detection" is the first test that fails
>> with a fast import error:
>> https://github.com/git/git/blob/78b384c29366e199741393e56030a8384110760d/t/t9801-git-p4-branch.sh#L110
>> 
>> fast-import crash report:
>>fast-import process: 77079
>>parent process : 77077
>>at 2016-05-14 07:48:40 +
>> 
>> fatal: offset beyond end of packfile (truncated pack?)
> 
> Hmm, does that suggest Eric's "explode them into loose instead of
> keeping a small pack" insufficient?  It sounds like that somebody
> wanted to read some data back from its packfile without knowing that
> the updated code may make it available in a loose object form, which
> would mean that somebody needs to be told about what is going on,
> namely, these objects are not in a half-written pack but are found
> as loose objects.

I think that is pretty much the problem. Here is what is happening:

1.  git-p4 imports all changelists for the "main" branch

2.  git-p4 starts to import a second branch and creates a fast-import
"progress checkpoint". This triggers:

--> fast-import.c: cycle_packfile
--> fast-import.c: end_packfile
--> fast-import.c: loosen_small_pack

At this point we have no packfile anymore.

3.  git-p4 sends the command "commit refs/remotes/p4/depot/branch2"
to fast-import in order to create a new branch. This triggers:

--> fast-import.c: parse_new_commit
--> fast-import.c: load_branch
--> fast-import.c: load_tree

"load_tree" calls "find_object" and the result has a "pack_id" of 0.
I think this indicates that the object is in the packfile. Shouldn't
the "pack_id" be "MAX_PACK_ID" instead?

myoe = find_object(sha1);
if (myoe && myoe->pack_id != MAX_PACK_ID) {

--> fast-import.c: gfi_unpack_entry

In "gfi_unpack_entry" this condition evaluates to "true":

struct packed_git *p = all_packs[oe->pack_id];
if (p == pack_data && p->pack_size < (pack_size + 20)) 

In the comment below Git thinks that the object is stored in the
packfile. This seems wrong but the flow continues:

--> sha1_filec: unpack_entry
--> sha1_filec: unpack_object_header
--> sha1_filec: use_pack

At this point fast-import dies with "offset beyond end of packfile 
(truncated pack?)".

I am no expert in "fast-import". Does anyone have a few hints how to
proceed?

Thanks,
Lars--
To unsubscribe from this list: send the line "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 v6 00/17] Port branch.c to use ref-filter's printing options

[Karthik Nayak]

On Tue, May 17, 2016 at 3:42 AM, Junio C Hamano  wrote:
>
> Karthik Nayak  writes:
>
> > This is part of unification of the commands 'git tag -l, git branch -l
> > and git for-each-ref'. This ports over branch.c to use ref-filter's
> > printing options.
> >
> > Initially posted here: $(gmane/279226). It was decided that this series
> > would follow up after refactoring ref-filter parsing mechanism, which
> > is now merged into master (9606218b32344c5c756f7c29349d3845ef60b80c).
>
> 9606218b?
>

I believe it should be 50cd83dca100174ba95baa9f6ed8426bce731ac2

>
> > Changes in this version:
> >
> > 1. Rebased on top of f307218 (t6302: simplify non-gpg cases).
>
>
>   $ git checkout f307218
>   $ git am 0001-ref-filter-implement-if.txt
>   Applying: ref-filter: implement %(if), %(then), and %(else) atoms
>   error: patch failed: Documentation/git-for-each-ref.txt:181
>   error: Documentation/git-for-each-ref.txt: patch does not apply
>   Patch failed at 0001 ref-filter: implement %(if), %(then), and %(else) atoms
>   The copy of the patch that failed is found in: .git/rebase-apply/patch
>   When you have resolved this problem, run "git am --continue".
>   If you prefer to skip this patch, run "git am --skip" instead.
>   To restore the original branch and stop patching, run "git am --abort".

Hello, sorry for the confusion, it's built on top of 'next' which contains
f307218 (t6302: simplify non-gpg cases). The merge conflict is due to the
commit made by you 1cca17df (Documentation: fix linkgit references).

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