Re: [PATCH 1/2] t7101, t7014: rename test files to indicate what that file is for

[Duy Nguyen]

On Tue, Feb 4, 2014 at 11:05 PM, Jonathan Nieder  wrote:
>>  t/t7101-reset-empty-subdirs.sh (new +x) | 63 
>> +
>>  t/t7101-reset.sh (gone) | 63 
>> -
>>  t/t7104-reset-hard.sh (new +x)  | 46 
>>  t/t7104-reset.sh (gone) | 46 
>
> Hm, summary incorporated in the diffstat.  Neat. :)

Yeah, it's from this patch [1]. Maybe I should give it another push,
then remove --summary from format-patch diffstat.

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

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

Junio C Hamano  writes:

> On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen  wrote:
>> No no. I found that duplicate, but I did not suggest removing it
>> because it is needed there..
>
> Hmph, if that is the case, we probably should make it the
> responsibility of the calling side to actually mark ce->flags with the
> bit (which would also mean the function must be renamed to make it
> clear that it does not mark).

After looking at the codepath that uses the record_intent_to_add()
before this patch, I am coming to the conclusion that it is the
right thing to do after all.  The code appears in this section:

if (!intent_only) {
if (index_path(ce->sha1, path, st, HASH_WRITE_OBJECT))
return error("unable to index file %s", path);
} else
record_intent_to_add(ce);

which tells (at least) me: "We are not adding the contents of this
path, so we do not run index_path(); instead we call this helper
function to set the object name in ce to represent an intent-to-add
entry".

So I'll rename it to set_object_name_for_intent_to_add_entry() or
something, restore that flag manipulation back to the caller, and
add another to the new caller, and requeue.

--
To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line

[David Kastrup]

Junio C Hamano  writes:

> David Kastrup  writes:
>
>> It's snake oil making debugging harder.
>
> OK, that is a sensible argument.
>
>>> This was fun ;-)
>>
>> At the expense of seriously impacting my motivation to do any further
>> code cleanup on Git.
>
> Well, I said it was "fun" because I was learning from a new person
> who haven't made much technical or code aethesitics discussion here
> so far.  If teaching others frustrates you and stops contributing to
> the project, that is a loss.

The implication of "Thanks for catching them, but this patch needs heavy
style fixes." is not one of learning.

> But the styles matter, as the known pattern in the existing code is
> what lets our eyes coast over unimportant details of the code while
> reviewing and understanding.  I tend to be pickier when reviewing code
> from new people who are going to make large contributions for exactly
> that reason---new blood bringing in new ideas is fine, but I'd want to
> see those new ideas backed by solid thiniking, and that means they may
> have to explain themselves to those who are new to their ideas.

Well, the point of stylistic decisions is exactly that they are not
individually "backed by solid thinking": if they were, one would not
require a style.

A pattern of
some loop {
  ...
  if (condition) {
code intimately related to condition;
continue;
  }
  break;
}

is somewhat awkward.  The superficially simpler

some loop {
  ...
  if (!condition)
break;
  code intimately related to condition;
}

separates related parts with a central loop exit.  Which maybe a tossup.
The former pattern of break; at the end of a loop, however, becomes
indispensible in the form

some loop {
  ...
  switch (...) {
various cases ending in either break; or continue;
  }
  break;
}

In this case, the break; before the end of the loop establishes the
statement "any commencement of the loop will be explicitly done using
continue;", particularly important since a break; inside of the switch
statement does not, without this help, break out of the loop.

It's a pattern that is transparent enough to be still preferable over
"crank out the goto already, chicken".

Is "if I have to use x in some situations anyway I may as well pick it
when there would be equivalent solutions" solid thinking?  Not really.
It's about choosing one's familiars.  Which ultimately boils down to
personal style.  And where the differences are not really significant
for understanding and _are_ a conscious expression rather than just an
accident, the thin line between "write in a way that does not go against
our style and/or good sense" and "write in the way I would have written
it" may be the line between fun and work.

-- 
David Kastrup

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

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

On Wed, Feb 5, 2014 at 3:48 PM, Duy Nguyen  wrote:
> No no. I found that duplicate, but I did not suggest removing it
> because it is needed there..

Hmph, if that is the case, we probably should make it the
responsibility of the calling side to actually mark ce->flags with the
bit (which would also mean the function must be renamed to make it
clear that it does not mark).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Duy Nguyen]

On Thu, Feb 6, 2014 at 1:25 AM, Junio C Hamano  wrote:
>> Yes, indeed.  I wonder why your new test did not notice it, though
>> ;-)
>
> ... and the answer turns out to be that it was not testing the right
> thing.  On top of that faulty version, this will fix it.

Yes, write-tree should test that well.

> Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes
> sense but a caller needs to be adjusted to drop the duplicated flag
> manipulation.

No no. I found that duplicate, but I did not suggest removing it
because it is needed there..

> @@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char 
> *path, struct stat *st,
> ce->ce_namelen = namelen;
> if (!intent_only)
> fill_stat_cache_info(ce, st);
> -   else
> -   ce->ce_flags |= CE_INTENT_TO_ADD;
>
> if (trust_executable_bit && has_symlinks)
> ce->ce_mode = create_ce_mode(st_mode);

A few lines down, there's ie_match_stat() call that will check
CE_INTENT_TO_ADD and returns "changed" immediately without looking at
stat data. If stat info is used, it may (not so sure) return "not
changed", the exit path is taken and mark_intent_to_add() is ignored.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[BUG] t9151: Unreliable test/test setup

[Martin Erik Werner]

Hi,

It appears that the last test in t9151-svn-mergeinfo.sh:

test_expect_failure 'everything got merged in the end' '
unmerged=$(git rev-list --all --not master) &&
[ -z "$unmerged" ]
'

reports "known breakage" or "breakage vanished" seemingly at random:

$ while true; do (cd t && sh t9151-svn-mergeinfo.sh | \
grep -q vanished && printf "f" || printf "b"); done
bbbbbffbffbbbffbfffbbbbffbbbfbbffbbfbfff

I would guess that it might not be the test itself that is unreliable,
but rather the svn setup done prior, looking at test logs:

(cd t && mkdir -p logs; i=0; \
while true; do sh t9151-svn-mergeinfo.sh --verbose 2>&1 | tee logs/cur \
| grep -q vanished && \
(printf "f" && mv logs/cur logs/fixed-"$i") || \
(printf "b" && mv logs/cur logs/broken-"$i"); \
i=$((i+1)); done)
bbffbff

the only consistent difference between broken and fixed seems to be in
the svn setup stage and more specifically the bit below, with r44
becoming different SHA1s in "broken" and "fixed" imports:

--- broken-02014-02-05 23:40:21.412967698 +0100
+++ fixed-2 2014-02-05 23:40:44.441536583 +0100
(...)
@@ -176,12 +176,12 @@
M   subdir/palindromes
 r43 = a671eec900764a4ab85a6166def3e0d30f1a2664 (refs/remotes/bugfix)
M   subdir/palindromes
-Couldn't find revmap for 
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/branches/bugfix/subdir
-Couldn't find revmap for 
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0/subdir
-W: Cannot find common ancestor between 
90411e1b2118e11664e368a24a1eaa5e8749d150 and 
fdb537791ee8ba532e49c3d5a34a30feeb87bd59. Ignoring merge info.
 Couldn't find revmap for 
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0
 Found merge parent (svn:mergeinfo prop): 
a671eec900764a4ab85a6166def3e0d30f1a2664
-r44 = a110dec28a4b152b394906b1303fbf19174f7d26 (refs/remotes/trunk)
+Couldn't find revmap for 
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/branches/bugfix/subdir
+Couldn't find revmap for 
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/tags/v1.0/subdir
+Found merge parent (svn:mergeinfo prop): 
fdb537791ee8ba532e49c3d5a34a30feeb87bd59
+r44 = 8b619659a5126105c0a9765b655b6a1add9db4c1 (refs/remotes/trunk)
 Checked out HEAD:
   
file:///home/arand/utv/git/git/t/trash%20directory.t9151-svn-mergeinfo/svnrepo/trunk
 r44
 ok 1 - load svn dump

Does anyone who is more familiar with the test know what's going on
here? Is there any way to fix it, or should the test maybe be disabled
completely for the time being?

--
Martin Erik Werner 
--
To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line

[David Kastrup]

Junio C Hamano  writes:

> David Kastrup  writes:
>
>> Junio C Hamano  writes:
>>
>>> which I think is the prevalent style in our codebase.  The same for
>>> the other loop we see in the new code below.
>>>
>>>  - avoid assignments in conditionals when you do not have to.
>>
>> commit a77a48c259d9adbe7779ca69a3432e493116b3fd
>> Author: Junio C Hamano 
>> Date:   Tue Jan 28 13:55:59 2014 -0800
>>
>> combine-diff: simplify intersect_paths() further
>> [...]
>>
>> +   while ((p = *tail) != NULL) {
>>
>> Because we can.
>
> Be reasonable.  You cannot sensibly rewrite it to
>
>   p = *tail;
> while (p) {
>   ...
>   p = *tail;
>   }
>
> when you do not know how ... part would evolve in the future.

The only unknown here is the potential presence of "continue;" in
... and that can be addressed by writing

for (p = *tail; p; p = *tail) {
   ...
}

However, that only makes sense where ... is rather large and diverse and
the assignment in question provides a unifying point.  In this case, the
loop is rather small and perfectly fits on one screen.  It turns out
that the assignment only serves for _obfuscating_ the various code
paths.  We have:

while ((p = *tail) != NULL) {
cmp = ((i >= q->nr)
   ? -1 : strcmp(p->path, q->queue[i]->two->path));

if (cmp < 0) {
/* p->path not in q->queue[]; drop it */
*tail = p->next;
free(p);
continue;
}

if (cmp > 0) {
/* q->queue[i] not in p->path; skip it */
i++;
continue;
}

hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
p->parent[n].mode = q->queue[i]->one->mode;
p->parent[n].status = q->queue[i]->status;

tail = &p->next;
i++;
}

While we could instead have:
  p = curr;
  while (p) {
cmp = ((i >= q->nr)
   ? -1 : strcmp(p->path, q->queue[i]->two->path));

if (cmp < 0) {
struct combine_diff_path *n = p->next;
/* p->path not in q->queue[]; drop it */
free(p);
p = *tail = n;
continue;
}

if (cmp > 0) {
/* q->queue[i] not in p->path; skip it */
i++;
continue;
}

hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
p->parent[n].mode = q->queue[i]->one->mode;
p->parent[n].status = q->queue[i]->status;

p = *(tail = &p->next);
i++;
}

Of course, it only makes limited sense to recheck p after the second if, so
it would be clearer to write

  p = curr;
  while (p) {
cmp = ((i >= q->nr)
   ? -1 : strcmp(p->path, q->queue[i]->two->path));

if (cmp < 0) {
struct combine_diff_path *n = p->next;
/* p->path not in q->queue[]; drop it */
free(p);
p = *tail = n;
continue;
}

if (cmp > 0) {
/* q->queue[i] not in p->path; skip it */
i++;
continue;
}

hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
p->parent[n].mode = q->queue[i]->one->mode;
p->parent[n].status = q->queue[i]->status;

p = *(tail = &p->next);
i++;
}

But that's sort of a red herring since the actual loop structure is
hidden in conditions where it does not belong.  (i >= q->nr) is a
_terminal_ condition.

So it's more like

p = curr;
while (p) {
if (i >= q->nr) {
*tail = NULL;
do {
struct combine_diff_path *n = p->next;
free(p);
p = n;
} while (p);
break;
}
cmp = strcmp(p->path, q->queue[i]->two->path));
if (cmp < 0) {
struct combine_diff_path *n = p->next;
/* p->path not in q->queue[]; drop it */
free(p);
p = *tail = n;
continue;
}
if (cmp == 0) {
hashcpy(p->parent[n].sha1, q->queue[i]->one->sha1);
p->parent[n].mode = q->queue[i]->one->mode;
p->parent[n].status = q->queue[i]->status;

p = *(tail = &p->next);
}
i++;
}

>   if ((p = *tail) != NULL) {
>   ...
>
> is a totally different issue.

Yes: it was just a matter of s

What's cooking in git.git (Feb 2014, #02; Wed, 5)

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

v1.9.0-rc3 is expected to happen this weekend or early next 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

--
[New Topics]

* aj/ada-diff-word-pattern (2014-02-05) 1 commit
 - userdiff: update Ada patterns

 Will merge to 'next' and then to 'master'.


* jk/makefile (2014-02-05) 16 commits
 - FIXUP
 - move LESS/LV pager environment to Makefile
 - Makefile: teach scripts to include make variables
 - FIXUP
 - Makefile: auto-build C strings from make variables
 - Makefile: drop *_SQ variables
 - FIXUP
 - Makefile: add c-quote helper function
 - Makefile: introduce sq function for shell-quoting
 - Makefile: always create files via make-var
 - Makefile: store GIT-* sentinel files in MAKE/
 - Makefile: prefer printf to echo for GIT-*
 - Makefile: use tempfile/mv strategy for GIT-*
 - Makefile: introduce make-var helper function
 - Makefile: fix git-instaweb dependency on gitweb
 - Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS


* ks/tree-diff-walk (2014-02-05) 4 commits
 - revision: convert to using diff_tree_sha1()
 - line-log: convert to using diff_tree_sha1()
 - tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1 with 
old=NULL
 - tree-diff: allow diff_tree_sha1 to accept NULL sha1

 Will merge to 'next'.


* nd/reset-intent-to-add (2014-02-05) 1 commit
 - reset: support "--mixed --intent-to-add" mode

 Will merge to 'next'.


* nd/tag-doc (2014-02-04) 1 commit
 - git-tag.txt:  for --contains is optional

 Will merge to 'next' and then to 'master'.


* nd/test-rename-reset (2014-02-04) 1 commit
 - t7101, t7014: rename test files to indicate what that file is for

 Will merge to 'next'.


* tb/repack-fix-renames (2014-02-05) 1 commit
 - repack.c: rename a few variables

 Perhaps unneeded, as the longer-term plan is to drop the codeblock
 this change touches.

 Will discard.


* tr/remerge-diff (2014-02-05) 6 commits
 - log --remerge-diff: show what the conflict resolution changed
 - merge-recursive: allow storing conflict hunks in index
 - Fold all merge diff variants into an enum
 - combine-diff: do not pass revs->dense_combined_merges redundantly
 - log: add a merge base inspection option
 - pretty: refactor add_merge_info() into parts
 (this branch uses tr/merge-recursive-index-only.)

 "log -p" output learns a new way to let users inspect a merge
 commit by showing the differences between the automerged result
 with conflicts the person who recorded the merge would have seen
 and the final conflict resolution that was recorded in the merge.

 RFC.


* ow/manpages-typofix (2014-02-05) 1 commit
 - Documentation: fix typos in man pages

 Various typofixes, all looked correct.

 Will merge to 'next' and then to 'master'.

--
[Stalled]

* po/everyday-doc (2014-01-27) 1 commit
 - Make 'git help everyday' work

 This may make the said command to emit something, but the source is
 not meant to be formatted into a manual pages to begin with, and
 also its contents are a bit stale.  It may be a good first step in
 the right direction, but needs more work to at least get the
 mark-up right before public consumption.

 Will hold.


* jk/branch-at-publish-rebased (2014-01-17) 5 commits
 - t1507 (rev-parse-upstream): fix typo in test title
 - implement @{publish} shorthand
 - branch_get: provide per-branch pushremote pointers
 - branch_get: return early on error
 - sha1_name: refactor upstream_mark

 Give an easier access to the tracking branches from "other" side in
 a triangular workflow by introducing B@{publish} that works in a
 similar way to how B@{upstream} does.

 Will hold.


* rb/merge-prepare-commit-msg-hook (2014-01-10) 4 commits
 - merge: drop unused arg from abort_commit method signature
 - merge: make prepare_to_commit responsible for write_merge_state
 - t7505: ensure cleanup after hook blocks merge
 - t7505: add missing &&

 Expose more merge states (e.g. $GIT_DIR/MERGE_MODE) to hooks that
 run during "git merge".  The log message stresses too much on one
 hook, prepare-commit-msg, but it would equally apply to other hooks
 like post-merge, I think.

 Waiting for a reroll.


* jl/submodule-recursive-checkout (2013-12-26) 5 commits
 - Teach checkout to recursively checkout submodules
 - submodule: teach unpack_trees() to update submodules
 - submodule: teach unpack_trees() to repopulate submodules
 - submodule: teach unpack_trees() to remove submodule contents
 - submodule: prepare for recursive checkout of submodules

 Expecting a reroll.


* jc/graph-post-root-gap (2013-12-30) 3 commits
 - WIP: document what we want at the end
 - graph: remove unused code a bit
 - graph: stuff the current commit into graph->colum

[ANNOUNCE] Git v1.8.5.4

[Junio C Hamano]

The latest maintenance release Git v1.8.5.4 is now available at
the usual places.

The release tarballs are found at:

http://code.google.com/p/git-core/downloads/list

and their SHA-1 checksums are:

cbf14318ee9652232489982bb2da15d2e5ebb580  git-1.8.5.4.tar.gz
6cfb7f23d2a3493d5b7657cc4558ff791294beb0  git-htmldocs-1.8.5.4.tar.gz
4ee26cf0d2db87b0be21192c4433359b6f38b217  git-manpages-1.8.5.4.tar.gz

The following public repositories all have a copy of the v1.8.5.4
tag and the maint branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

Also, http://www.kernel.org/pub/software/scm/git/ has copies of the
release tarballs.

Git v1.8.5.4 Release Notes
==

Fixes since v1.8.5.4


 * "git fetch --depth=0" was a no-op, and was silently ignored.
   Diagnose it as an error.

 * Remote repository URL expressed in scp-style host:path notation are
   parsed more carefully (e.g. "foo/bar:baz" is local, "[::1]:/~user" asks
   to connect to user's home directory on host at address ::1.

 * SSL-related options were not passed correctly to underlying socket
   layer in "git send-email".

 * "git commit -v" appends the patch to the log message before
   editing, and then removes the patch when the editor returned
   control. However, the patch was not stripped correctly when the
   first modified path was a submodule.

 * "git mv A B/", when B does not exist as a directory, should error
   out, but it didn't.

 * When we figure out how many file descriptors to allocate for
   keeping packfiles open, a system with non-working getrlimit() could
   cause us to die(), but because we make this call only to get a
   rough estimate of how many is available and we do not even attempt
   to use up all file descriptors available ourselves, it is nicer to
   fall back to a reasonable low value rather than dying.

 * "git log --decorate" did not handle a tag pointed by another tag
   nicely.

 * "git add -A" (no other arguments) in a totally empty working tree
   used to emit an error.

 * There is no reason to have a hardcoded upper limit of the number of
   parents for an octopus merge, created via the graft mechanism, but
   there was.

 * The implementation of 'git stash $cmd "stash@{...}"' did not quote
   the stash argument properly and left it split at IFS whitespace.

 * The documentation to "git pull" hinted there is an "-m" option
   because it incorrectly shared the documentation with "git merge".

Also contains typofixes, documentation updates and trivial code clean-ups.



Changes since v1.8.5.3 are as follows:

Jens Lehmann (1):
  commit -v: strip diffs and submodule shortlogs from the commit message

Johannes Schindelin (1):
  Remove the line length limit for graft files

Johannes Sixt (2):
  git_connect: remove artificial limit of a remote command
  git_connect: factor out discovery of the protocol and its parts

Junio C Hamano (4):
  get_max_fd_limit(): fall back to OPEN_MAX upon getrlimit/sysconf failure
  Documentation: exclude irrelevant options from "git pull"
  Documentation: "git pull" does not have the "-m" option
  Git 1.8.5.4

Nguyễn Thái Ngọc Duy (2):
  clone,fetch: catch non positive --depth option value
  add: don't complain when adding empty project root

Roman Kagan (1):
  git-svn: workaround for a bug in svn serf backend

Thomas Rast (3):
  send-email: pass Debug to Net::SMTP::SSL::new
  send-email: --smtp-ssl-cert-path takes an argument
  send-email: set SSL options through IO::Socket::SSL::set_client_defaults

Torsten Bögershausen (8):
  t5601: remove clear_ssh, refactor setup_ssh_wrapper
  t5601: add tests for ssh
  git fetch-pack: add --diag-url
  t5500: add test cases for diag-url
  git fetch: support host:/~repo
  git_connect(): refactor the port handling for ssh
  connect.c: refactor url parsing
  git_connect(): use common return point

brian m. carlson (1):
  log: properly handle decorations with chained tags

Øystein Walle (1):
  stash: handle specifying stashes with $IFS

--
To unsubscribe from this list: send the line "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 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Junio C Hamano]

Kirill Smelkov  writes:

> On Wed, Feb 05, 2014 at 11:42:41AM -0800, Junio C Hamano wrote:
>> Kirill Smelkov  writes:
>> 
>> > I agree object data should be immutable for good. The only thing I'm 
>> > talking
>> > about here is mode, which is parsed from a tree buffer and is stored in
>> > separate field:
>> 
>> Ah, I do not see any problem in that case, then.
>> 
>> Thanks.
>
> Thanks, that simplifies things for me.

Surely.

Be careful when traversing N-trees in parallel---you may have to
watch out for the entry ordering rules that sorts the following
paths in the order shown:

a
a-b
a/b
a_b

Inside a single tree, you cannot have 'a' and 'a/b' at the same
time, but one tree may have 'a' (without 'a/b') while another one
may have 'a/b' (without 'a'), and walking them in parallel has
historically been one source of funny 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

[RFH] hackday and GSoC topic suggestions

[Jeff King]

This Saturday I'm going to be attending a Git hackday held by Bloomberg
in New York. The participants will be eager C coders who have experience
using git, but not contributing to it. As somebody who has read The
Mythical Man Month, I don't expect huge productivity, but I'm hoping to
do some bug triage and fixes, and maybe get some people involved who
might join the community. I'm hoping some of the folks will have
features _they_ want to work on, and I can help get them started. But
I'd like to have a list of potential projects to direct people towards
as a backup.

On a similar note, the GSoC application deadline is Feb 14th. I am
happy to be admin again and put together the application, but we will
need an idea page. I'll set up a page to collect them, but in the
meantime, please dump any ideas/discussion in this thread.

Below is a list of features / bugs that I am taking to the hackday. The
bug list was collected by grepping the mailing list for items without
responses. They haven't been triaged at all, so I'm sure some of them
are "not a bug" or "won't fix".  But the triage process is part of what
we'll be doing on Saturday.

I don't think the list below and the potential GSoC list really have any
overlap, as the project scales are completely different. But I'd be
happy to take suggestions for either.

---
features:

 - negative refspecs
   http://thread.gmane.org/gmane.comp.version-control.git/240997/focus=241019

 - optionally remove tempfiles on failed pack-objects (especially when
   we get ENOSPC)
   http://article.gmane.org/gmane.comp.version-control.git/241466

 - previewing "git pull"
   http://article.gmane.org/gmane.comp.version-control.git/236732

bugs:

 - branch.*.merge interpreted too strictly by tracking logic
   http://article.gmane.org/gmane.comp.version-control.git/241582

 - relative core.worktree is resolved from symlink and not its target
   http://article.gmane.org/gmane.comp.version-control.git/241519

 - Branch rename breaks local downstream branches
   http://article.gmane.org/gmane.comp.version-control.git/241228

 - git clone on out-of-space device causes incorrect errors
   http://article.gmane.org/gmane.comp.version-control.git/241206

 - commit-msg hook and merges
   http://article.gmane.org/gmane.comp.version-control.git/241203

 - inconsistent include behaviour for core.sharedRepository
   http://article.gmane.org/gmane.comp.version-control.git/241277

 - Rebase options '--whitespace=fix' and '--keep-empty' are incompatible
   http://article.gmane.org/gmane.comp.version-control.git/238055

 - git stash doesn't use --index as default
   http://article.gmane.org/gmane.comp.version-control.git/235892

 - git describe --contains --abbrev=0  doesn't work as expected
   http://article.gmane.org/gmane.comp.version-control.git/236707

 - using git commit-tree with "-F -" adds trailing newlines
   http://article.gmane.org/gmane.comp.version-control.git/236583

 - Pull and fetch don't honor `--progress` flag
   http://thread.gmane.org/gmane.comp.version-control.git/236257/focus=236262

 - Unexpected outputs of git pull on stdout v.s. stderr
   http://article.gmane.org/gmane.comp.version-control.git/235716

 - MERGE_HEAD lost with git checkout master
   http://article.gmane.org/gmane.comp.version-control.git/233806

 - git stash doesn't always save work dir as-is: bug?
   http://article.gmane.org/gmane.comp.version-control.git/234153

 - Well-past commit dates unsupported
   http://article.gmane.org/gmane.comp.version-control.git/236827

 - we do not handle integer overflow in commit/author timestamps
   [no reference, but something I have noticed; we should probably
   return a sentinel "0" rather than a random overflow value, and
   we should probably detect and warn in git-fsck]

 - git stash does not work when directory is replaced by a symlink to itself
   http://article.gmane.org/gmane.comp.version-control.git/236798

 - rebase not recovering gracefully from repack error
   http://article.gmane.org/gmane.comp.version-control.git/234261

 - (broken ?) output of "git diff --color-word"
   http://article.gmane.org/gmane.comp.version-control.git/237370

 - git filter-branch does not make tree replacements permanent
   http://article.gmane.org/gmane.comp.version-control.git/220931

 - 'git show' gives duplicate errors for ambiguous args
   http://article.gmane.org/gmane.comp.version-control.git/205023

 - (git commit --patch --message $MESSAGE) disallows hunk editing
   http://article.gmane.org/gmane.comp.version-control.git/208213

 - filter-branch --parent-filter in bare repository
   http://article.gmane.org/gmane.comp.version-control.git/203619

insanely hard bugs:

 - shallow clones over http
   http://article.gmane.org/gmane.comp.version-control.git/223682
--
To unsubscribe from this list: send the line "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] Documentation: fix typos in man pages

[Junio C Hamano]

Øystein Walle  writes:

> Signed-off-by: Øystein Walle 
> ---
>
> In July I sent in some typo fixes but it halted in a discussion on UK
> vs. US English and so forth ($gmane/231331). There were some actual typo
> fixes in there though (in addition to the "opinionated typo fixes").
>
> Powering up my old laptop for the first time in months I noticed that I
> had them idling in a branch, and that incidentally none of them has been
> fixed. Hopefully I've managed to seperate the genuine typo fixes from
> the rest.

Wonderful.

All of them looked fine. I however am already deep in today's
integration cycle, so it will appear in the public branches
tomorrow.

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

[PATCH] Documentation: fix typos in man pages

[Øystein Walle]

Signed-off-by: Øystein Walle 
---

In July I sent in some typo fixes but it halted in a discussion on UK
vs. US English and so forth ($gmane/231331). There were some actual typo
fixes in there though (in addition to the "opinionated typo fixes").

Powering up my old laptop for the first time in months I noticed that I
had them idling in a branch, and that incidentally none of them has been
fixed. Hopefully I've managed to seperate the genuine typo fixes from
the rest.

 Documentation/git-clone.txt   | 2 +-
 Documentation/git-diff.txt| 2 +-
 Documentation/gitcli.txt  | 2 +-
 Documentation/githooks.txt| 2 +-
 Documentation/gitweb.conf.txt | 4 ++--
 Documentation/user-manual.txt | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 4987857..bf3dac0 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -208,7 +208,7 @@ objects from the source repository into a pack in the 
cloned repository.
 --separate-git-dir=::
Instead of placing the cloned repository where it is supposed
to be, place the cloned repository at the specified directory,
-   then make a filesytem-agnostic Git symbolic link to there.
+   then make a filesystem-agnostic Git symbolic link to there.
The result is Git repository can be separated from working
tree.
 
diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
index 33fbd8c..56fb7e5 100644
--- a/Documentation/git-diff.txt
+++ b/Documentation/git-diff.txt
@@ -44,7 +44,7 @@ two blob objects, or changes between two files on disk.
commit relative to the named .  Typically you
would want comparison with the latest commit, so if you
do not give , it defaults to HEAD.
-   If HEAD does not exist (e.g. unborned branches) and
+   If HEAD does not exist (e.g. unborn branches) and
 is not given, it shows all staged changes.
--staged is a synonym of --cached.
 
diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index 3f33ca5..1c3e109 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -28,7 +28,7 @@ arguments.  Here are the rules:
they can be disambiguated by placing `--` between them.
E.g. `git diff -- HEAD` is, "I have a file called HEAD in my work
tree.  Please show changes between the version I staged in the index
-   and what I have in the work tree for that file". not "show difference
+   and what I have in the work tree for that file", not "show difference
between the HEAD commit and the work tree as a whole".  You can say
`git diff HEAD --` to ask for the latter.
 
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index d48bf4d..d954bf6 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -251,7 +251,7 @@ three parameters:
 
  - the name of the ref being updated,
  - the old object name stored in the ref,
- - and the new objectname to be stored in the ref.
+ - and the new object name to be stored in the ref.
 
 A zero exit from the update hook allows the ref to be updated.
 Exiting with a non-zero status prevents 'git-receive-pack'
diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index db4154f..952f503 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -630,13 +630,13 @@ need to set this element to empty list i.e. `[]`.
 
 override::
If this field has a true value then the given feature is
-   overriddable, which means that it can be configured
+   overridable, which means that it can be configured
(or enabled/disabled) on a per-repository basis.
 +
 Usually given "" is configurable via the `gitweb.`
 config variable in the per-repository Git configuration file.
 +
-*Note* that no feature is overriddable by default.
+*Note* that no feature is overridable by default.
 
 sub::
Internal detail of implementation.  What is important is that
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index 248dcab..d4f9804 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -3795,7 +3795,7 @@ like so:
 $ git update-index filename
 -
 
-but to avoid common mistakes with filename globbing etc, the command
+but to avoid common mistakes with filename globbing etc., the command
 will not normally add totally new entries or remove old entries,
 i.e. it will normally just update existing cache entries.
 
-- 
1.8.5

--
To unsubscribe from this list: send the line "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.c: always honor the filename case

[Torsten Bögershausen]

On 2014-02-03 23.11, Reuben Hawkins wrote:
>
>
>
> On Mon, Feb 3, 2014 at 2:21 PM, Torsten Bögershausen  > wrote:
>
> []
> > So to summarize, when fast-import uses strncmp_icase (what fast-import 
> does now) import on a repository where ignorecase=true is wrong.  My patch, 
> "fast-import.c: always honor the filename case" fixes this.  Can you verify?
> >
> > Thanks in advance,
> > Reuben
> >
> Yes, I can verify. My feeling is that
> a) the fast-export should generate the rename the other way around.
>Would that be feasable ?
>
>
> I *think* this is feasible.  I did try this, and it worked, but I didn't like 
> the idea of having to fix all the exporters.  I know about hg-export and 
> svn-export, but I assume there are more that I don't know about, and maybe 
> even other tools out there none of us know about, which would also have to be 
> fixed in the same way.  As such, I decided fixing fast-export isn't really 
> the right thing to do...I don't really think fast-export is broken to begin 
> with.  I'm hoping there is a way to fix ignorecase such that it doesn't 
> create this type of problem with this...
>
> M 100644 :1 Filename.txt
> D FileName.txt
>
> ..maybe by very carefully identifying when ignorecase should apply and when 
> it shouldn't (I'm still trying to sort that out, the docs on ignorecase 
> aren't specific).
>
> But for what it's worth, to "fix" fast-export, I added a check in 
> builtin/fast-export.c in the function depth_first before all the other checks 
> so it would always make diff_filepair->status == 'D' the lesser when not 
> equal...something like this (I'm not looking at the code now, so this may 
> *not* be what I did)...
>
> if (a->status != b->status) {
>   if (a->status == 'D') return -1;
>   if (b->status == 'D') return 1;
> }
>
> /Reuben
>  
>
>Or generate a real rename ?
>
>
> A rename may also work, but it may not.  And that would also require fixing 
> all other exporters.  If I understand the docs well enough, a rename would be 
> done like so...
>
> R Filename.txt FileName.txt
>
> I think in the ignorecase=true situation, case folding would happen and this 
> would be a nop like this...
>
> R Filename.txt Filename.txt
>
> ...Right?  I haven't tested this, but I *suspect* the result would be to not 
> rename the file when ignorecase=true...I definitely think it's worth a try 
> just to know the result, but this fixes the ignorecase problem in fast-import 
> by passing a requirement to all the fast-exporters...a semi-artificial 
> requirement created because ignorecase *could* be true, but may not be.
> /Reuben
>  
>
>   (I'm not using fast-export or import myself)
>
> b)  As a workaround, does it help if you manually set core.ignorecase 
> false ?
>
>
> Yes, this works.  It makes a single step clone, git clone hg::..., into a 
> multi step process like this...
>
> $ mkdir test
> $ git init
> $ git config core.ignorecase false
> $ git remote add origin hg::whatever
> $ git fetch origin
> $ git reset --hard origin/master
> $ git branch --set-upstream-to=origin/master master
>
> That isn't a too big deal for people fluent in GIT (if you only have to do it 
> once and wrote it down too maybe).  It works, just not ideally and it's easy 
> to get burned on because git clone sort-of works, it just doesn't truly 
> clone.  The resulting cloned repo can be mangled by case folding.
>
> Typically, unless somebody explains the multi step process to everybody, some 
> people will have master with commit xx and others will have the exact 
> same master with commit yy.  Some will have Filename.txt instead of 
> FileName.txt way back in history.  Merging those branches is a mess.
>
> So setting core.ignorecase=flase does work, it's just a bit cumbersome.  My 
> fingers really want to just type git clone hg::whatever and I hope to get a 
> true 'clone' as in an exact, identical copy on all machines regardless of 
> filesystem.  If GIT wants to do case folding after that I suppose it would be 
> fine.  Maybe I'm expecting too much, but I've been under the impression for 
> years that a clone of any git repo will have all the exact same commit id's.
> /Reuben
>
>
> c)  Does it help to use git-hg-remote ? (could be another workaround)
>
>
> Yes, sorry, I guess I wasn't clear on that point.  That is what I'm using. 
> /Reuben
>
>
> And no,  50906e04e8f48215b0 does not include any test cases.
> (try git show 50906e04e8)
>
> This is only a short answer, I can prepare a longer answer about 
> ignorecase the next days.
> /Torsten
>
>
> Thank you!  That would be very helpful.  I'm still trying to wrap my head 
> around what ignorecase really needs to do, when and where it needs to do it 
> and what it shouldn't do.  I suspect ignorecase is touching too many code 
> paths and needs to be reined in a bit.
>
> I'm also wondering if it's possible to test a bunch of situations in 'make 
> test

Re: [Bug] branch.*.merge interpreted too strictly by tracking logic

[Junio C Hamano]

Jeff King  writes:

>  Did your report come
> out of a real case, or was it just something you noticed?

Some git-wrappers (like "repo") are reported to muck with the
configuration files.
--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Jeff King]

On Wed, Feb 05, 2014 at 01:08:36PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > ... So the fact that this
> > bug exists doesn't really produce any user-visible behavior, and
> > hopefully post-release we would drop the code entirely, and the test
> > would have no reason to exist.
> 
> Heh, thanks, and I agree with the reasoning for the longer-term
> direction.  Perhaps I can/should hold it off that minimal fix-up
> patch from -rc3, then?  I am on the fence but I already started my
> today's integration cycle _with_ the fix merged to 'master', so...

I would say leave the fix for -rc3, but not worry about the test.

-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] repack.c: rename and unlink pack file if it exists

[Junio C Hamano]

Jeff King  writes:

> ... So the fact that this
> bug exists doesn't really produce any user-visible behavior, and
> hopefully post-release we would drop the code entirely, and the test
> would have no reason to exist.

Heh, thanks, and I agree with the reasoning for the longer-term
direction.  Perhaps I can/should hold it off that minimal fix-up
patch from -rc3, then?  I am on the fence but I already started my
today's integration cycle _with_ the fix merged to 'master', so...
--
To unsubscribe from this list: send the line "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] branch.*.merge interpreted too strictly by tracking logic

[Jeff King]

On Wed, Feb 05, 2014 at 01:05:04PM -0800, Junio C Hamano wrote:

> > I don't recall us ever doing anything after that. I don't have a problem
> > with making it work, of course, but I am not sure if it is really a bug.
> 
> Once people get used to us being extra nice in some places, other
> less nice places start looking more and more like bugs. It is an
> unfortunate fact of life, but fixing them up is a good thing for
> users.  As long as we can make those less nice places nicer
> uniformly without bending backwards or introducing unnecessary
> ambiguities, that is, and I think this one can be done without
> such downsides.

Oh, absolutely, and I do not think we are breaking anything to start
handling it better (my "I don't have a problem..." above). But I guess I
am doubting that people are actually doing this at all now. I'd expect
most people to have the config set automatically by "branch" or
"checkout", or to use "branch --set-upstream-to". Did your report come
out of a real case, or was it just something you noticed?

-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] branch.*.merge interpreted too strictly by tracking logic

[Junio C Hamano]

Jeff King  writes:

> Is it legal to put an unqualified ref there? A wise man once said[1]:
>
>   > Actually, it is broken in a lot of places. for-each-ref relies on
>   > the same code as "git status", "git checkout", etc, which will all
>   > fail to display tracking info. I believe the same code is also used
>   > for updating tracking branches on push. So I'm not sure if it was
>   > ever intended to be a valid setting.
>
>   It wasn't.  Some places may accept them gracefully by either being
>   extra nice or by accident.
>
> I don't recall us ever doing anything after that. I don't have a problem
> with making it work, of course, but I am not sure if it is really a bug.

Once people get used to us being extra nice in some places, other
less nice places start looking more and more like bugs. It is an
unfortunate fact of life, but fixing them up is a good thing for
users.  As long as we can make those less nice places nicer
uniformly without bending backwards or introducing unnecessary
ambiguities, that is, and I think this one can be done without
such downsides.
--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Torsten Bögershausen]

On 2014-02-05 21.31, Junio C Hamano wrote:
> Jeff King  writes:
> 
>> The minimal fix you posted below does make sense to me as a stopgap, and
>> we can look into dropping the code entirely during the next cycle. It
>> would be nice to have a test to cover this case, though.
> 
> Sounds sensible.  Run "repack -a -d" once, and then another while
> forcing it to be single threaded, or something?
I can put a test case on my todo list,
and thanks for the minimal patch.

--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Jeff King]

On Wed, Feb 05, 2014 at 12:57:14PM -0800, Junio C Hamano wrote:

> > ...does not seem to fail, and it does not seem to leave any cruft in
> > place. So maybe I am misunderstanding the thing the patch is meant to
> > fix. Is it that we simply do not replace the pack in this instance?
> 
> Yes.  Not just the command finishing OK, but the packfile left by
> the first repack needs to be left intact. [...]

Thanks for the explanation. Having looked at this now, I'm thinking a
test may not be worth the trouble. Due to 1190a1ac, we effectively don't
care whether we get the old pack or the new one. So the fact that this
bug exists doesn't really produce any user-visible behavior, and
hopefully post-release we would drop the code entirely, and the test
would have no reason to exist.

> We could use test-chmtime to reset the timestamp of the packfile
> generated by the first repack to somewhere reasonably old and then
> rerun the repack to see that it is a different file, which may be
> more portable than inspecting the inum of the packfile.

Yeah, I think that would work. But it sounds like we also need a
filesystem in which rename() does not overwrite. So the test would not
be portable.

-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] repack.c: rename and unlink pack file if it exists

[Junio C Hamano]

Jeff King  writes:

> On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>> 
>> > The minimal fix you posted below does make sense to me as a stopgap, and
>> > we can look into dropping the code entirely during the next cycle. It
>> > would be nice to have a test to cover this case, though.
>> 
>> Sounds sensible.  Run "repack -a -d" once, and then another while
>> forcing it to be single threaded, or something?
>
> Certainly that's the way to trigger the code, but doing this:
>
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index b45bd1e..6647ba1 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts 
> only are kept' '
>   git reflog expire --expire=$test_tick --expire-unreachable=$test_tick 
> --all &&
>   git repack -a -d &&
>   git cat-file -t $H1
> - '
> +'
> +
> +test_expect_success 'repack can handle generating the same pack again' '
> + git -c pack.threads=1 repack -ad &&
> + git -c pack.threads=1 repack -ad
> +'
>  
>  test_done
>  
>
> ...does not seem to fail, and it does not seem to leave any cruft in
> place. So maybe I am misunderstanding the thing the patch is meant to
> fix. Is it that we simply do not replace the pack in this instance?

Yes.  Not just the command finishing OK, but the packfile left by
the first repack needs to be left intact.  One bug was that after
learning that a new packfile  needs to be installed, the command
did not check existing .git/objects/pack/pack-.{pack,idx}, but
checked .git/objects/pack/.{pack,idx}, deciding that there is
nothing that needs to be saved by renaming with s|pack-|old-|.  This
would have caused it to rename the new packfile left by pack-object
at .git/objects/pack/.tmp-$pid-pack-.{pack,idx} directly to the
final .git/objects/pack/pack-.{pack,idx}, which would work only
on filesystems that allow renaming over an existing file.

Another bug fixed by Torsten was in the codepath to roll the rename
back from .git/objects/pack/old-.{pack,idx} to the original (the
command tried to rename .git/objects/pack/old-pack-.{pack,idx}
which would not have been the ones it renamed), but because of the
earlier bug, it would never have triggered in the first place.

> I guess we would have to generate a pack with the identical set of
> objects, then, but somehow different in its pack parameters (perhaps
> turning off deltas?).

Unfortunately, that would trigger different codepath on v1.9-rc0 and
later with 1190a1ac (pack-objects: name pack files after trailer
hash, 2013-12-05), as it is likely not to name the packfiles the
same.

We could use test-chmtime to reset the timestamp of the packfile
generated by the first repack to somewhere reasonably old and then
rerun the repack to see that it is a different file, which may be
more portable than inspecting the inum of the packfile.
--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Jeff King]

On Wed, Feb 05, 2014 at 03:37:40PM -0500, Jeff King wrote:

> > Sounds sensible.  Run "repack -a -d" once, and then another while
> > forcing it to be single threaded, or something?
> 
> Certainly that's the way to trigger the code, but doing this:
> [...]
> ...does not seem to fail, and it does not seem to leave any cruft in
> place. So maybe I am misunderstanding the thing the patch is meant to
> fix. Is it that we simply do not replace the pack in this instance?
> 
> I guess we would have to generate a pack with the identical set of
> objects, then, but somehow different in its pack parameters (perhaps
> turning off deltas?).

Here's a more robust test that actually checks the pack contents:

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index b45bd1e..c18a318 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -164,5 +164,17 @@ test_expect_success 'objects made unreachable by grafts 
only are kept' '
git cat-file -t $H1
'
 
+test_expect_success 'repack can handle generating the same pack again' '
+   show_deltas() {
+   git rev-list --objects --all --reflog |
+   git cat-file --batch-check="%(objectname) %(deltabase) %(rest)"
+   }
+   git -c pack.threads=1 repack -adf --window=0 &&
+   show_deltas >no-deltas &&
+   git -c pack.threads=1 repack -adf --window=10 &&
+   show_deltas >deltas &&
+   ! test_cmp no-deltas deltas
+'
+
 test_done
 

which _also_ does not fail. And then I realized it is because of
1190a1ac, which gives these two separate names.

So I am not sure if it is even possible to trigger the bug in a
meaningful way at this point.

-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] branch.*.merge interpreted too strictly by tracking logic

[Jeff King]

On Tue, Feb 04, 2014 at 02:49:16PM -0800, Junio C Hamano wrote:

> Let's tell these branches that they are both supposed to be building
> on top of 'master'.
> 
> : gitster track/master; git config branch.foo.remote .
> : gitster track/master; git config branch.foo.merge refs/heads/master
> : gitster track/master; git config branch.bar.remote .
> : gitster track/master; git config branch.bar.merge master
> 
> The difference between the two is that 'foo' spells the @{upstream}
> branch in full (which is the recommended practice), while 'bar' is
> loose and asks for 'master'.

Is it legal to put an unqualified ref there? A wise man once said[1]:

  > Actually, it is broken in a lot of places. for-each-ref relies on
  > the same code as "git status", "git checkout", etc, which will all
  > fail to display tracking info. I believe the same code is also used
  > for updating tracking branches on push. So I'm not sure if it was
  > ever intended to be a valid setting.

  It wasn't.  Some places may accept them gracefully by either being
  extra nice or by accident.

I don't recall us ever doing anything after that. I don't have a problem
with making it work, of course, but I am not sure if it is really a bug.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/121671
--
To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line

[Junio C Hamano]

David Kastrup  writes:

> It's snake oil making debugging harder.

OK, that is a sensible argument.

>> This was fun ;-)
>
> At the expense of seriously impacting my motivation to do any further
> code cleanup on Git.

Well, I said it was "fun" because I was learning from a new person
who haven't made much technical or code aethesitics discussion here
so far.  If teaching others frustrates you and stops contributing to
the project, that is a loss.

But the styles matter, as the known pattern in the existing code is
what lets our eyes coast over unimportant details of the code while
reviewing and understanding.  I tend to be pickier when reviewing
code from new people who are going to make large contributions for
exactly that reason---new blood bringing in new ideas is fine, but
I'd want to see those new ideas backed by solid thiniking, and that
means they may have to explain themselves to those who are new to
their ideas.

--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Jeff King]

On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The minimal fix you posted below does make sense to me as a stopgap, and
> > we can look into dropping the code entirely during the next cycle. It
> > would be nice to have a test to cover this case, though.
> 
> Sounds sensible.  Run "repack -a -d" once, and then another while
> forcing it to be single threaded, or something?

Certainly that's the way to trigger the code, but doing this:

diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index b45bd1e..6647ba1 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts 
only are kept' '
git reflog expire --expire=$test_tick --expire-unreachable=$test_tick 
--all &&
git repack -a -d &&
git cat-file -t $H1
-   '
+'
+
+test_expect_success 'repack can handle generating the same pack again' '
+   git -c pack.threads=1 repack -ad &&
+   git -c pack.threads=1 repack -ad
+'
 
 test_done
 

...does not seem to fail, and it does not seem to leave any cruft in
place. So maybe I am misunderstanding the thing the patch is meant to
fix. Is it that we simply do not replace the pack in this instance?

I guess we would have to generate a pack with the identical set of
objects, then, but somehow different in its pack parameters (perhaps
turning off deltas?).

-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] repack.c: rename and unlink pack file if it exists

[Junio C Hamano]

Jeff King  writes:

> On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:
>
>>  * Somehow this came to my private mailbox without Cc to list, so I
>>am forwarding it.
>> 
>>I think with 1190a1ac (pack-objects: name pack files after
>>trailer hash, 2013-12-05), repacking the same set of objects may
>>have less chance of producing colliding names, especially if you
>>are on a box with more than one core, but it still would be a
>>good idea to get this part right in the upcoming release.
>
> Actually, since 1190a1ac, if you have repacked and gotten the same pack
> name, then you do not have to do any rename dance at all; you can throw
> away what you just generated because you know that it is byte-for-byte
> identical.
>
> You could collide with a pack created by an older version of git that
> used the original scheme, but that is quite unlikely (on the order of
> 2^-160).

Yes, so in that sense this is not so urgent, but I'm tempted to
split the original patch into two and merge only the first one to
'master' before -rc3 (see below).  The renaming of the variables
added enough noise to cause me fail to spot a change mixed within.

-- >8 --
From: Torsten Bögershausen 
Date: Sun, 2 Feb 2014 16:09:56 +0100

When a repo was fully repacked, and is repacked again, we may run
into the situation that "new" packfiles have the same name as
already existing ones (traditionally packfiles have been named after
the list of names of objects in them, so repacking all the objects
in a single pack would have produced a packfile with the same name).

The logic is to rename the existing ones into filename like
"old-XXX", create the new ones and then remove the "old-" ones.
When something went wrong in the middle, this sequence is rolled
back by renaming the "old-" files back.

The renaming into "old-" did not work as intended, because
file_exists() was done on "XXX", not "pack-XXX".  Also when rolling
back the change, the code tried to rename "old-pack-XXX" but the
saved ones are named "old-XXX", so this couldn't have worked.

Signed-off-by: Torsten Bögershausen 
Signed-off-by: Junio C Hamano 
---
 builtin/repack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index bca7710..fe31577 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -260,7 +260,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname, *fname_old;
-   fname = mkpathdup("%s/%s%s", packdir,
+   fname = mkpathdup("%s/pack-%s%s", packdir,
item->string, exts[ext]);
if (!file_exists(fname)) {
free(fname);
@@ -337,7 +337,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
char *fname;
-   fname = mkpath("%s/old-pack-%s%s",
+   fname = mkpath("%s/old-%s%s",
packdir,
item->string,
exts[ext]);
-- 
1.9-rc2-217-g24a8b2e


--
To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line

[Junio C Hamano]

David Kastrup  writes:

> Junio C Hamano  writes:
>
>> which I think is the prevalent style in our codebase.  The same for
>> the other loop we see in the new code below.
>>
>>  - avoid assignments in conditionals when you do not have to.
>
> commit a77a48c259d9adbe7779ca69a3432e493116b3fd
> Author: Junio C Hamano 
> Date:   Tue Jan 28 13:55:59 2014 -0800
>
> combine-diff: simplify intersect_paths() further
> [...]
>
> +   while ((p = *tail) != NULL) {
>
> Because we can.

Be reasonable.  You cannot sensibly rewrite it to

p = *tail;
while (p) {
...
p = *tail;
}

when you do not know how ... part would evolve in the future.

if ((p = *tail) != NULL) {
...

is a totally different issue.

--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Junio C Hamano]

Jeff King  writes:

> The minimal fix you posted below does make sense to me as a stopgap, and
> we can look into dropping the code entirely during the next cycle. It
> would be nice to have a test to cover this case, though.

Sounds sensible.  Run "repack -a -d" once, and then another while
forcing it to be single threaded, or something?
--
To unsubscribe from this list: send the line "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 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Kirill Smelkov]

On Wed, Feb 05, 2014 at 11:42:41AM -0800, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > I agree object data should be immutable for good. The only thing I'm talking
> > about here is mode, which is parsed from a tree buffer and is stored in
> > separate field:
> 
> Ah, I do not see any problem in that case, then.
> 
> Thanks.

Thanks, that simplifies things for me.
--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Jeff King]

On Wed, Feb 05, 2014 at 12:06:41PM -0800, Junio C Hamano wrote:

> > Actually, since 1190a1ac, if you have repacked and gotten the same pack
> > name, then you do not have to do any rename dance at all; you can throw
> > away what you just generated because you know that it is byte-for-byte
> > identical.
> >
> > You could collide with a pack created by an older version of git that
> > used the original scheme, but that is quite unlikely (on the order of
> > 2^-160).
> 
> Yes, so in that sense this is not so urgent, but I'm tempted to
> split the original patch into two and merge only the first one to
> 'master' before -rc3 (see below).  The renaming of the variables
> added enough noise to cause me fail to spot a change mixed within.

That sounds very sensible. The only reason I did not follow-up 1190a1ac
immediately with a patch to drop the rename code was that it is a
sensitive area, and I wanted to be very sure there would be no other
fallouts. And then of course I didn't get around to it yet. But
following the same logic, trying to do it during -rc would be a terrible
idea. :)

The minimal fix you posted below does make sense to me as a stopgap, and
we can look into dropping the code entirely during the next cycle. It
would be nice to have a test to cover this case, though.

-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] repack.c: rename and unlink pack file if it exists

[Junio C Hamano]

... and this is the other half that is supposed to be only about
renaming variables.

-- >8 --
From: Torsten Bögershausen 
Date: Sun, 2 Feb 2014 16:09:56 +0100
Subject: [PATCH] repack.c: rename a few variables

Rename the variables to match what they are used for: fname for the
final name of the new packfile, fname_old for the name of the
existing one, and fname_tmp for the temporary name pack-objects
created the new packfile in.

Signed-off-by: Torsten Bögershausen 
Signed-off-by: Junio C Hamano 
---
 builtin/repack.c | 22 +++---
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index fe31577..3b01353 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -316,33 +316,33 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
/* Now the ones with the same name are out of the way... */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
-   char *fname, *fname_old;
+   char *fname, *fname_tmp;
struct stat statbuffer;
fname = mkpathdup("%s/pack-%s%s",
packdir, item->string, exts[ext]);
-   fname_old = mkpathdup("%s-%s%s",
+   fname_tmp = mkpathdup("%s-%s%s",
packtmp, item->string, exts[ext]);
-   if (!stat(fname_old, &statbuffer)) {
+   if (!stat(fname_tmp, &statbuffer)) {
statbuffer.st_mode &= ~(S_IWUSR | S_IWGRP | 
S_IWOTH);
-   chmod(fname_old, statbuffer.st_mode);
+   chmod(fname_tmp, statbuffer.st_mode);
}
-   if (rename(fname_old, fname))
-   die_errno(_("renaming '%s' failed"), fname_old);
+   if (rename(fname_tmp, fname))
+   die_errno(_("renaming '%s' into '%s' failed"), 
fname_tmp, fname);
free(fname);
-   free(fname_old);
+   free(fname_tmp);
}
}
 
/* Remove the "old-" files */
for_each_string_list_item(item, &names) {
for (ext = 0; ext < 2; ext++) {
-   char *fname;
-   fname = mkpath("%s/old-%s%s",
+   char *fname_old;
+   fname_old = mkpath("%s/old-%s%s",
packdir,
item->string,
exts[ext]);
-   if (remove_path(fname))
-   warning(_("removing '%s' failed"), fname);
+   if (remove_path(fname_old))
+   warning(_("removing '%s' failed"), fname_old);
}
}
 
-- 
1.9-rc2-217-g24a8b2e

--
To unsubscribe from this list: send the line "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 13/13] move LESS/LV pager environment to Makefile

[Jeff King]

On Wed, Feb 05, 2014 at 02:23:50PM -0500, Jeff King wrote:

> On Wed, Feb 05, 2014 at 01:08:57PM -0500, Jeff King wrote:
> 
> > +quote() {
> > +   echo "$1" | sed 's/\\//g; s/"/\\"/'
> > +}
> 
> This of course has the same "/g" bug as the earlier patch in the series.
> 
> I was tempted to pull the function out into script/lib-c.sh, so that
> both can source it, but perhaps that is overkill.

Also, this (and the other spot) should probably be using "printf"
instead of echo for portability (i.e., the config variables might have
backslashes in them).

-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 12/13] Makefile: teach scripts to include make variables

[Jeff King]

On Wed, Feb 05, 2014 at 11:26:58AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> >  define cmd_munge_script
> >  $(RM) $@ $@+ && \
> > +{ \
> > +includes="$(filter MAKE/%.sh,$^)"; \
> > +if ! test -z "$$includes"; then \
> > +   cat $$includes; \
> > +fi && \
> >  sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
> >  -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
> > --e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
> >  -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
> >  -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> >  -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
> >  -e $(BROKEN_PATH_FIX) \
> >  -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
> >  -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
> > -$@.sh >$@+
> > +$@.sh; \
> > +} >$@+
> >  endef
> 
> Sorry, but I am not quite sure what is going on here.
> [...]
> And then after emitting that piece, we start processing the *.sh
> source file, replacing she-bang line?

Yes, there is a bug here. The intent was to end up with:

  #!/bin/sh
  [include snippets]
  [the actual script]

but of course I bungled that, because #!/bin/sh is part of the file
already, and our snippets should not come before. If we take this
technique to its logical conclusion, the sed replacement goes away
entirely, and we end up with something like:

  %: %.sh MAKE/SHELL_SHEBANG.sh
  { cat $(filter MAKE/%.sh,$^) && sed 1d $<; } >$@+
  chmod +x $@+
  mv $@+ $@

  MAKE/SHELL_SHEBANG.sh: MAKE/SHELL_SHEBANG
  echo "#!$(head -1 $<)" >$@+ &&
  mv $@+ $@

I.e., the shebang line simply becomes the first snippet.

> >  create_virtual_base() {
> > sz0=$(wc -c <"$1")
> > -   @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> > +   $MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> > sz1=$(wc -c <"$1")
> 
> This would mean that after this mechanism is extensively employed
> throughout our codebase, any random environment variable the user
> has whose name happens to begin with "MAKE_" will interfere with us
> (rather, we will override such a variable while we run).  Having to
> carve out our own namespace in such a way is OK, but we would want
> to see that namespace somewhat related to the name of our project,
> not to the name of somebody else's like "make", no?

Yes, it probably makes sense to carve out our own namespace. I kind of
dislike the use of environment variables at all, though, just because it
really bleeds through into how you write the script (as opposed to just
replacing like we do now, or in C, where a snippet defining a
preprocessor macro is just fine).

An alternative is that we could do something like:

  %: %.sh
  script/mkshellscript $^ >$@+ &&
  chmod +x $@+ &&
  mv $@+ $@

and then have mkshellscript look like (and this could obviously be
inline in the Makefile, but I think it's worth pulling it out to avoid
the quoting hell):

  #!/bin/sh

  main=$1; shift

  sed_quote() {
  sed 's/\\//g; s/|/\\|/g'
  }

  # generate a sed expression that will replace tokens; if we are given
  # the file MAKE/FOO.sh, the expression will replace  @@FOO@@ with the
  # contents of that file
  sed_replace() {
  for var in "$@"; do
  key=${i#MAKE/}
  key=${key%.sh}
  printf 's|@@%s@@|%s|g' "$key" "$(sed_quote <$var)"
  done
  }

  sed "$(sed_replace "$@")" <"$main"

-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 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Junio C Hamano]

Kirill Smelkov  writes:

> I agree object data should be immutable for good. The only thing I'm talking
> about here is mode, which is parsed from a tree buffer and is stored in
> separate field:

Ah, I do not see any problem in that case, then.

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 12/13] Makefile: teach scripts to include make variables

[Junio C Hamano]

Jeff King  writes:

>  define cmd_munge_script
>  $(RM) $@ $@+ && \
> +{ \
> +includes="$(filter MAKE/%.sh,$^)"; \
> +if ! test -z "$$includes"; then \
> + cat $$includes; \
> +fi && \
>  sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
>  -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
> --e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
>  -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
>  -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
>  -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
>  -e $(BROKEN_PATH_FIX) \
>  -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
>  -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
> -$@.sh >$@+
> +$@.sh; \
> +} >$@+
>  endef

Sorry, but I am not quite sure what is going on here.

 - if $includes does not exist, cat $includes will barf but that is
   OK;

 - if $includes is empty, there is no point running cat so that is
   OK;

 - if $includes is not empty, we want to cat.

And then after emitting that piece, we start processing the *.sh
source file, replacing she-bang line?

> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index fffa3c7..627d289 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -285,7 +285,7 @@ clear_local_git_env() {
>  # remove lines from $1 that are not in $2, leaving only common lines.
>  create_virtual_base() {
>   sz0=$(wc -c <"$1")
> - @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> + $MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
>   sz1=$(wc -c <"$1")

This would mean that after this mechanism is extensively employed
throughout our codebase, any random environment variable the user
has whose name happens to begin with "MAKE_" will interfere with us
(rather, we will override such a variable while we run).  Having to
carve out our own namespace in such a way is OK, but we would want
to see that namespace somewhat related to the name of our project,
not to the name of somebody else's like "make", no?

>  
>   # If we do not have enough common material, it is not
> diff --git a/script/mksh b/script/mksh
> new file mode 100644
> index 000..d41e77a
> --- /dev/null
> +++ b/script/mksh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +name=$1; shift
> +printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"
--
To unsubscribe from this list: send the line "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 13/13] move LESS/LV pager environment to Makefile

[Jeff King]

On Wed, Feb 05, 2014 at 01:08:57PM -0500, Jeff King wrote:

> +quote() {
> + echo "$1" | sed 's/\\//g; s/"/\\"/'
> +}

This of course has the same "/g" bug as the earlier patch in the series.

I was tempted to pull the function out into script/lib-c.sh, so that
both can source it, but perhaps that is overkill.

-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 11/13] Makefile: auto-build C strings from make variables

[Jeff King]

On Wed, Feb 05, 2014 at 11:17:13AM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > diff --git a/script/mkcstring b/script/mkcstring
> > new file mode 100644
> > index 000..c01f430
> > --- /dev/null
> > +++ b/script/mkcstring
> > @@ -0,0 +1,18 @@
> > +#!/bin/sh
> > +
> > +name=$1; shift
> > +
> > +c_quote() {
> > +   sed 's/\\//g; s/"/\\"/'
> 
> No 'g' for the second one?

That's a bug. Thanks for catching.

I tested most of these changes manually, but I missed this one by only
testing a value with a single quote. At one point I had introduced:

  $(eval $(call make-var,FOO,debug variable,$(FOO)))

so you could do "make MAKE/FOO" and "make MAKE/foo-string.c", but I did
not include it in the series. Adding a test suite to our Makefile kind
of seems like overkill, but as it gets complex, maybe some simple sanity
checks are worthwhile (not part of the regular test suite, but maybe
just a "./test-make" script to make sure it behaves). I dunno.

-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 09/13] Makefile: add c-quote helper function

[Jeff King]

On Wed, Feb 05, 2014 at 11:13:24AM -0800, Junio C Hamano wrote:

> > +# Quote the value as C string inside a shell string. Good for passing 
> > strings
> > +# on the command line via "-DFOO=$(call # scq,$(FOO))".
> 
> "call # scq"???

Whoops. Bad rewrapping of the comment. It should obviously just be

  -DFOO=$(call scq,$(FOO))

-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 11/13] Makefile: auto-build C strings from make variables

[Junio C Hamano]

Jeff King  writes:

> diff --git a/script/mkcstring b/script/mkcstring
> new file mode 100644
> index 000..c01f430
> --- /dev/null
> +++ b/script/mkcstring
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +
> +name=$1; shift
> +
> +c_quote() {
> + sed 's/\\//g; s/"/\\"/'

No 'g' for the second one?

> +}
> +
> +cat <<-EOF
> +#ifndef MAKE_${name}_H
> +#define MAKE_${name}_H
> +
> +/* Auto-generated by mkcstring */
> +
> +#define MAKE_${name} "$(c_quote)"
> +
> +#endif /* MAKE_${name}_H */
> +EOF
> diff --git a/version.c b/version.c
> index 6106a80..f68a93b 100644
> --- a/version.c
> +++ b/version.c
> @@ -1,8 +1,10 @@
>  #include "git-compat-util.h"
>  #include "version.h"
>  #include "strbuf.h"
> +#include "MAKE/USER-AGENT-string.h"
> +#include "MAKE/VERSION-string.h"
>  
> -const char git_version_string[] = GIT_VERSION;
> +const char git_version_string[] = MAKE_VERSION;
>  
>  const char *git_user_agent(void)
>  {
> @@ -11,7 +13,7 @@ const char *git_user_agent(void)
>   if (!agent) {
>   agent = getenv("GIT_USER_AGENT");
>   if (!agent)
> - agent = GIT_USER_AGENT;
> + agent = MAKE_USER_AGENT;
>   }
>  
>   return agent;
--
To unsubscribe from this list: send the line "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 10/13] Makefile: drop *_SQ variables

[Junio C Hamano]

Again, Yay!
--
To unsubscribe from this list: send the line "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 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Kirill Smelkov]

On Wed, Feb 05, 2014 at 09:36:55AM -0800, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > Only, before I clean things up, I'd like to ask - would the following
> > patch be accepted
> >
> >  8< ---
> > diff --git a/tree-walk.c b/tree-walk.c
> > index 79dba1d..4dc86c7 100644
> > --- a/tree-walk.c
> > +++ b/tree-walk.c
> > @@ -37,7 +37,7 @@ static void decode_tree_entry(struct tree_desc *desc, 
> > const char *buf, unsigned
> >  
> > /* Initialize the descriptor entry */
> > desc->entry.path = path;
> > -   desc->entry.mode = mode;
> > +   desc->entry.mode = canon_mode(mode);
> > desc->entry.sha1 = (const unsigned char *)(path + len);
> >  }
> >  
> > diff --git a/tree-walk.h b/tree-walk.h
> > index ae04b64..ae7fb3a 100644
> > --- a/tree-walk.h
> > +++ b/tree-walk.h
> > @@ -16,7 +16,7 @@ struct tree_desc {
> >  static inline const unsigned char *tree_entry_extract(struct tree_desc 
> > *desc, const char **pathp, unsigned int *modep)
> >  {
> > *pathp = desc->entry.path;
> > -   *modep = canon_mode(desc->entry.mode);
> > +   *modep = desc->entry.mode;
> > return desc->entry.sha1;
> >  }
> >  8< ---
> >  
> > ?
> 
> Doesn't desc point into and walks over the data we read from the
> tree object directly?
> 
> We try to keep (tree|commit)->buffer intact and that is done pretty
> deliberately.  While you are walking a tree or parsing a commit,
> somebody else, perhaps called indirectly by a helper function you
> call, may call lookup_object() for the same object, get the copy
> that has already been read and start using it.  This kind of change
> will introduce bugs that are hard to debug unless it is done very
> carefully (e.g. starting from making tree.buffer into a const pointer
> and propagating constness throughout the system), which might not be
> worth the pain.

I agree object data should be immutable for good. The only thing I'm talking
about here is mode, which is parsed from a tree buffer and is stored in
separate field:

 8<  tree-walk.h
struct name_entry {
const unsigned char *sha1;
const char *path;
unsigned int mode;  <---
};

struct tree_desc {
const void *buffer;
struct name_entry entry;
unsigned int size;
};

 8<  tree-walk.c
const char *get_mode(const char *str, unsigned int *modep)
{ ... } /* parses symbolic mode from tree buffer to uint */

void decode_tree_entry(struct tree_desc *desc, const char *buf, 
unsigned long size)
{
const char *path;
unsigned int mode, len;

...
path = get_mode(buf, &mode);

/* Initialize the descriptor entry */
desc->entry.path = path;
desc->entry.mode = mode;<---
desc->entry.sha1 = (const unsigned char *)(path + len);


so we are not talking about modifying object buffers here. All I'm
asking is about canonicalizing _already_ parsed and copied mode on the
fly.

Does that change anything?


Sorry, if I'm maybe misunderstanding something, and thanks,
Kirill
--
To unsubscribe from this list: send the line "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 08/13] Makefile: introduce sq function for shell-quoting

[Junio C Hamano]

Jeff King  writes:

> Since we have recently abolished the prohibition on $(call)
> in our Makefile, we can use it to factor out the repeated
> shell-quoting we do. There are two upsides:
>
>   1. It is much more readable than inline calls to
>  $(subst ','\'').
>
>   2. It is short enough that we can do away with the _SQ
>  variants of many variables (note that we do not do this
>  just yet, as there is a little more cleanup needed
>  first).

Yay.

>  But
> many instances are not really any more readable (e.g., see the first
> hunk below).
> ...
>  .PHONY: install-perl-script install-sh-script install-python-script
>  install-sh-script: $(SCRIPT_SH_INS)
> - $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
> + $(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))

Hmph, I do not see it as bad as the "make-var", which forces you to
say $(eval $(call ...)); this $(call sq, ...) is fairly readable.

--
To unsubscribe from this list: send the line "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 09/13] Makefile: add c-quote helper function

[Junio C Hamano]

Jeff King  writes:

> We have to c-quote strings coming from Makefile variables
> when we pass them to the compiler via "-D". Now that we can
> use $(call) in our Makefile, we can factor out the quoting
> to make things easier to read. We can also apply it more
> consistently, as there were many spots that should have been
> C-quoting but did not. For example:
>
>   make prefix='foo\bar'
>
> would produce an exec_cmd.o with a broken prefix.
>
> Signed-off-by: Jeff King 
> ---
>  Makefile | 58 +-
>  1 file changed, 29 insertions(+), 29 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index 868872f..b1c3fb4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1568,6 +1568,17 @@ endif
>  sqi = $(subst ','\'',$1)
>  sq = '$(call sqi,$1)'
>  
> +# usage: $(call cq,CONTENTS)
> +#
> +# Quote the value as appropriate for a C string literal.
> +cq = "$(subst ",\",$(subst \,\\,$1))"
> +
> +# usage: $(call scq,CONTENTS)
> +#
> +# Quote the value as C string inside a shell string. Good for passing strings
> +# on the command line via "-DFOO=$(call # scq,$(FOO))".

"call # scq"???

> +scq = $(call sq,$(call cq,$1))
> +
>  # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
>  #
>  # Create a rule to write $CONTENTS (which should come from a make variable)
> @@ -1617,28 +1628,17 @@ LIB_OBJS += $(COMPAT_OBJS)
>  # Quote for C
>  
>  ifdef DEFAULT_EDITOR
> -DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
> -DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
> -
> -BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
> +BASIC_CFLAGS += -DDEFAULT_EDITOR=$(call scq,$(DEFAULT_EDITOR))
>  endif
>  
>  ifdef DEFAULT_PAGER
> -DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
> -DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
> -
> -BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
> +BASIC_CFLAGS += -DDEFAULT_PAGER=$(call scq,$(DEFAULT_PAGER))
>  endif
>  
>  ifdef SHELL_PATH
> -SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
> -SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
> -
> -BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
> +BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
>  endif
>  
> -GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
> -GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
>  $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
>  
>  ifdef DEFAULT_HELP_FORMAT
> @@ -1723,9 +1723,9 @@ strip: $(PROGRAMS) git$X
>  
>  git.sp git.s git.o: MAKE/PREFIX
>  git.sp git.s git.o: EXTRA_CPPFLAGS = \
> - '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> - '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> - '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
> + -DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
> + -DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
> + -DGIT_INFO_PATH=$(call scq,$(infodir_relative))
>  
>  git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
> @@ -1735,14 +1735,14 @@ help.sp help.s help.o: common-cmds.h
>  
>  builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
> - '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
> - '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
> - '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
> + -DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
> + -DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
> + -DGIT_INFO_PATH=$(call scq,$(infodir_relative))
>  
>  version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
>  version.sp version.s version.o: EXTRA_CPPFLAGS = \
> - '-DGIT_VERSION="$(GIT_VERSION)"' \
> - '-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
> + -DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
> + -DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
>  
>  $(BUILT_INS): git$X
>   $(QUIET_BUILT_IN)$(RM) $@ && \
> @@ -2020,25 +2020,25 @@ endif
>  
>  exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/PREFIX
>  exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
> - '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
> - '-DBINDIR="$(bindir_relative_SQ)"' \
> - '-DPREFIX="$(prefix_SQ)"'
> + -DGIT_EXEC_PATH=$(call scq,$(gitexecdir)) \
> + -DBINDIR=$(call scq,$(bindir_relative)) \
> + -DPREFIX=$(call scq,$(prefix))
>  
>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: MAKE/PREFIX
>  builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
> - -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
> + -DDEFAULT_GIT_TEMPLATE_DIR=$(call scq,$(template_dir))
>  
>  config.sp config.s config.o: MAKE/PREFIX
>  config.sp config.s config.o: EXTRA_CPPFLAGS = \
> - -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
> + -DETC_GITCONFIG=$(call scq,$(ETC_GITCONFIG))
>  
>  attr.sp attr.s attr.o: MAKE/PREFIX
>  attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
> - -DETC_GITATTRIBUTES='"$(ET

Re: [PATCH 06/13] Makefile: store GIT-* sentinel files in MAKE/

[Junio C Hamano]

Jeff King  writes:

> This patch moves all of the generated GIT-* files into
> MAKE/*, with one exception: GIT-VERSION-FILE. This could be
> moved along with the rest, but there is a reasonable chance
> that there are some out-of-tree scripts that may peek at it
> (whereas things like GIT-CFLAGS are purely internal, and we
> update all internal references).

OK.  We do have internal references to include the version file to
Makefiles in subdirectories, which we could adjust if we wanted to.
I tend to agree that leaving it there would be a safer thing to do,
but at the same time I think it is OK to move it if we wanted to.
The third parties need to adjust and they are capable of adjusting.

Thanks.  The changes look good.  I do not have a strong opinion on
the name, but I do agree a separate directory unclutters things in a
very good way.

> Signed-off-by: Jeff King 
> ---
> I'm not married to the name "MAKE", but I do think the separate
> directory is a win for simplicity and avoiding repetition (as you can
> see in the diffstat). Other suggestions welcome.
>
>  .gitignore|  8 ---
>  MAKE/.gitignore   |  1 +
>  Makefile  | 66 
> +--
>  perl/Makefile | 10 
>  t/perf/run|  2 +-
>  t/test-lib.sh |  2 +-
>  t/valgrind/analyze.sh |  4 ++--
>  7 files changed, 42 insertions(+), 51 deletions(-)
>  create mode 100644 MAKE/.gitignore
>
> diff --git a/.gitignore b/.gitignore
> index b5f9def..586a067 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,11 +1,3 @@
> -/GIT-BUILD-OPTIONS
> -/GIT-CFLAGS
> -/GIT-LDFLAGS
> -/GIT-PREFIX
> -/GIT-PERL-DEFINES
> -/GIT-PYTHON-VARS
> -/GIT-SCRIPT-DEFINES
> -/GIT-USER-AGENT
>  /GIT-VERSION-FILE
>  /bin-wrappers/
>  /git
> diff --git a/MAKE/.gitignore b/MAKE/.gitignore
> new file mode 100644
> index 000..72e8ffc
> --- /dev/null
> +++ b/MAKE/.gitignore
> @@ -0,0 +1 @@
> +*
> diff --git a/Makefile b/Makefile
> index 60dc53b..7fecdf1 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1564,10 +1564,10 @@ endif
>  # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
>  #
>  # Create a rule to write $CONTENTS (which should come from a make variable)
> -# to GIT-$FN, but only if not already there. This can be used to create a
> +# to MAKE/$FN, but only if not already there. This can be used to create a
>  # dependency on a Makefile variable. Prints $DESC to the user.
>  define make-var
> -GIT-$1: FORCE
> +MAKE/$1: FORCE
>   @VALUE='$$(subst ','\'',$3)'; \
>   if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
>   echo >&2 "* new $2"; \
> @@ -1658,7 +1658,7 @@ all:: profile-clean
>  endif
>  endif
>  
> -all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) 
> GIT-BUILD-OPTIONS
> +all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) 
> MAKE/BUILD-OPTIONS
>  ifneq (,$X)
>   $(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter 
> %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || 
> $(RM) '$p';)
>  endif
> @@ -1714,25 +1714,25 @@ strip: $(PROGRAMS) git$X
>  #   dependencies here will not need to change if the force-build
>  #   details change some day.
>  
> -git.sp git.s git.o: GIT-PREFIX
> +git.sp git.s git.o: MAKE/PREFIX
>  git.sp git.s git.o: EXTRA_CPPFLAGS = \
>   '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>   '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
>   '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
> -git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
> +git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>   $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
>   $(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
>  
>  help.sp help.s help.o: common-cmds.h
>  
> -builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
> +builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>   '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>   '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
>   '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
>  
> -version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
> +version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
>  version.sp version.s version.o: EXTRA_CPPFLAGS = \
>   '-DGIT_VERSION="$(GIT_VERSION)"' \
>   '-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
> @@ -1773,12 +1773,12 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
>  $@.sh >$@+
>  endef
>  
> -$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
> +$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
>   $(QUIET_GEN)$(cmd_munge_script) && \
>   chmod +x $@+ && \
>   mv $@+ $@
>  
> -$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
> +$(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
>   $(QUIET_GEN)$(cmd_munge_script) && \
>   mv $@+ $@
>  
> @@ -1797,14 +1797,14 @@ perl/PM.stamp: FORCE
>   { cmp $@+ $@ >/dev/null 2>/d

Re: [PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Junio C Hamano]

All four looked sensible; will queue.  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: bash completion patch

[Junio C Hamano]

Matthieu Moy  writes:

> 乙酸鋰  writes:
>
>> add --recurse-submodules
>
> Thanks for the patch, but it cannot be included as-is.
>
> Please, read Documentation/SubmittingPatches in Git's source tree. In
> particular, the signed-off-by part. Also, don't use attachments to send
> you patches (git send-email can help) and don't forget to Cc Junio if
> you think your patch is ready for inclusion.

Heh, thanks.  Everybody seems to think anything they send out to the
list is ready for inclusion, so the last part may not be a piece of
advice that is practically very useful, though ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

Junio C Hamano  writes:

> Duy Nguyen  writes:
>
>> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>>> Junio C Hamano  writes:
>>> 
>>> > While I do not have any problem with adding an optional "keep lost
>>> > paths as intent-to-add entries" feature, I am not sure why this has
>>> > to be so different from the usual add-cache-entry codepath.  The
>>> > if/elseif chain you are touching inside this loop does:
>>> >
>>> >  - If the tree you are resetting to has something at the path
>>> >(which is different from the current index, obviously), create
>>> >a cache entry to represent that state from the tree and stuff
>>> >it in the index;
>>> >
>>> >  - Otherwise, the tree you are resetting to does not have that
>>> >path.  We used to say "remove it from the index", but now we have
>>> >an option to instead add it as an intent-to-add entry.
>>> >
>>> > So, why doesn't the new codepath do exactly the same thing as the
>>> > first branch of the if/else chain and call add_cache_entry but with
>>> > a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
>>> > in "git add -N" better, I would think, no?
>>> 
>>> In other words, something along this line, perhaps?
>>
>> 
>>
>> Yes. But you need something like this on top to actually set
>> CE_INTENT_TO_ADD
>
> Yes, indeed.  I wonder why your new test did not notice it, though
> ;-)

... and the answer turns out to be that it was not testing the right
thing.  On top of that faulty version, this will fix it.

Your suggestion to move CE_INTENT_TO_ADD to mark-intent-to-add makes
sense but a caller needs to be adjusted to drop the duplicated flag
manipulation.

 read-cache.c | 3 +--
 t/t7102-reset.sh | 6 ++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 325d193..5b8102a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -584,6 +584,7 @@ void mark_intent_to_add(struct cache_entry *ce)
unsigned char sha1[20];
if (write_sha1_file("", 0, blob_type, sha1))
die("cannot create an empty blob in the object database");
+   ce->ce_flags |= CE_INTENT_TO_ADD;
hashcpy(ce->sha1, sha1);
 }
 
@@ -613,8 +614,6 @@ int add_to_index(struct index_state *istate, const char 
*path, struct stat *st,
ce->ce_namelen = namelen;
if (!intent_only)
fill_stat_cache_info(ce, st);
-   else
-   ce->ce_flags |= CE_INTENT_TO_ADD;
 
if (trust_executable_bit && has_symlinks)
ce->ce_mode = create_ce_mode(st_mode);
diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index 642920a..bc0846f 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -539,6 +539,12 @@ test_expect_success 'reset -N keeps removed files as 
intent-to-add' '
echo new-file >new-file &&
git add new-file &&
git reset -N HEAD &&
+
+   tree=$(git write-tree) &&
+   git ls-tree $tree new-file >actual &&
+   >expect &&
+   test_cmp expect actual &&
+
git diff --name-only >actual &&
echo new-file >expect &&
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: [PATCH 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Junio C Hamano]

Kirill Smelkov  writes:

> Only, before I clean things up, I'd like to ask - would the following
> patch be accepted
>
>  8< ---
> diff --git a/tree-walk.c b/tree-walk.c
> index 79dba1d..4dc86c7 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -37,7 +37,7 @@ static void decode_tree_entry(struct tree_desc *desc, const 
> char *buf, unsigned
>  
>   /* Initialize the descriptor entry */
>   desc->entry.path = path;
> - desc->entry.mode = mode;
> + desc->entry.mode = canon_mode(mode);
>   desc->entry.sha1 = (const unsigned char *)(path + len);
>  }
>  
> diff --git a/tree-walk.h b/tree-walk.h
> index ae04b64..ae7fb3a 100644
> --- a/tree-walk.h
> +++ b/tree-walk.h
> @@ -16,7 +16,7 @@ struct tree_desc {
>  static inline const unsigned char *tree_entry_extract(struct tree_desc 
> *desc, const char **pathp, unsigned int *modep)
>  {
>   *pathp = desc->entry.path;
> - *modep = canon_mode(desc->entry.mode);
> + *modep = desc->entry.mode;
>   return desc->entry.sha1;
>  }
>  8< ---
>  
> ?

Doesn't desc point into and walks over the data we read from the
tree object directly?

We try to keep (tree|commit)->buffer intact and that is done pretty
deliberately.  While you are walking a tree or parsing a commit,
somebody else, perhaps called indirectly by a helper function you
call, may call lookup_object() for the same object, get the copy
that has already been read and start using it.  This kind of change
will introduce bugs that are hard to debug unless it is done very
carefully (e.g. starting from making tree.buffer into a const pointer
and propagating constness throughout the system), which might not be
worth the pain.


--
To unsubscribe from this list: send the line "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 12/13] Makefile: teach scripts to include make variables

[Jeff King]

The current scheme for getting build-time variables into a
shell script is to munge the script with sed, and stick the
munged variable into a special sentinel file so that "make"
knows about the dependency.

Instead, we can combine both functions by generating a shell
snippet with our value, and then "building" shell scripts by
concatenating their snippets. "make" then handles the
dependency automatically, and it's easy to generate tighter
dependencies.

We demonstrate here by moving the "DIFF" substitution into
its own snippet, which lets us rebuild only the single
affected file when it changes.

Signed-off-by: Jeff King 
---
The astute reader will notice that:

  MAKE_DIFF='diff'
  $MAKE_DIFF ...

that we end up with is not _quite_ the same as replacing "$MAKE_DIFF" in
the actual script text with "diff" at build-time. In particular, the way
that whitespace and quotes are treated is different. I'm not sure what
we would want to do here. Calling "eval" would work. Or we could have
the snippet produce a function, rather than a variable, like:

  DIFF() {
  diff "$@"
  }

 Makefile| 18 +++---
 git-sh-setup.sh |  2 +-
 script/mksh |  4 
 3 files changed, 20 insertions(+), 4 deletions(-)
 create mode 100644 script/mksh

diff --git a/Makefile b/Makefile
index 203171d..ad3100d 100644
--- a/Makefile
+++ b/Makefile
@@ -1598,6 +1598,11 @@ MAKE/%-string.h: MAKE/% script/mkcstring
$(subst -,_,$*) <$< >$@+ && \
mv $@+ $@
 
+MAKE/%.sh: MAKE/% script/mksh
+   $(QUIET_GEN)$(SHELL_PATH) script/mksh \
+   $(subst -,_,$*) <$< >$@+ && \
+   mv $@+ $@
+
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -1734,7 +1739,6 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
 
 $(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
:$(SHELL_PATH)\
-   :$(DIFF)\
:$(GIT_VERSION)\
:$(localedir)\
:$(NO_CURL)\
@@ -1743,18 +1747,24 @@ $(eval $(call make-var,SCRIPT-DEFINES,script 
parameters,\
:$(gitwebdir)\
:$(PERL_PATH)\
 ))
+$(eval $(call make-var,DIFF,diff command,$(DIFF)))
 define cmd_munge_script
 $(RM) $@ $@+ && \
+{ \
+includes="$(filter MAKE/%.sh,$^)"; \
+if ! test -z "$$includes"; then \
+   cat $$includes; \
+fi && \
 sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
 -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
--e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
 -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
 -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
 -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
 -e $(BROKEN_PATH_FIX) \
 -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
 -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
-$@.sh >$@+
+$@.sh; \
+} >$@+
 endef
 
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
@@ -1766,6 +1776,8 @@ $(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
 
+git-sh-setup: MAKE/DIFF.sh
+
 git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
  $(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst ., 
,$(GIT_VERSION) \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index fffa3c7..627d289 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -285,7 +285,7 @@ clear_local_git_env() {
 # remove lines from $1 that are not in $2, leaving only common lines.
 create_virtual_base() {
sz0=$(wc -c <"$1")
-   @@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
+   $MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
sz1=$(wc -c <"$1")
 
# If we do not have enough common material, it is not
diff --git a/script/mksh b/script/mksh
new file mode 100644
index 000..d41e77a
--- /dev/null
+++ b/script/mksh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+name=$1; shift
+printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 13/13] move LESS/LV pager environment to Makefile

[Jeff King]

We set the LESS and LV variables to sensible defaults if
they are not already set. However, the code is brittle.
There is no easy way to change the defaults at compile time,
and we have to duplicate the code in git-sh-setup and in
pager.c.

Let's turn it into a normal Makefile knob instead.

Signed-off-by: Jeff King 
---
Bits of this patch were liberally stolen from Junio's weather-balloon
patch. All bugs are mine. :)

Note that setup_pager_env in the C code still does do a little parsing,
since we need to have the name of each variable in a separate buffer to
pass to getenv().  I chose to do it this way so that we could introduce
a standard "mkcarray" helper that could be used in other places. But it
would also be easy to do all of that parsing at compile time and produce
an array of structs like:

  { "LESS", "LESS=-FRSX" },
  { "LV", "LV=-c" }

 Makefile| 22 +-
 git-sh-setup.sh |  9 +
 pager.c | 34 +++---
 script/mkcarray | 25 +
 4 files changed, 74 insertions(+), 16 deletions(-)
 create mode 100644 script/mkcarray

diff --git a/Makefile b/Makefile
index ad3100d..fff8e72 100644
--- a/Makefile
+++ b/Makefile
@@ -342,6 +342,14 @@ all::
 # Define DEFAULT_HELP_FORMAT to "man", "info" or "html"
 # (defaults to "man") if you want to have a different default when
 # "git help" is called without a parameter specifying the format.
+#
+# Define PAGER_ENV to a SP separated VAR=VAL pairs to define
+# default environment variables to be passed when a pager is spawned, e.g.
+#
+#PAGER_ENV = LESS=-FRSX LV=-c
+#
+# to say "export LESS=-FRSX (and LV=-c) if the environment variable
+# LESS (and LV) is not set, respectively".
 
 GIT-VERSION-FILE: FORCE
@$(SHELL_PATH) ./GIT-VERSION-GEN
@@ -1505,6 +1513,10 @@ ifeq ($(PYTHON_PATH),)
 NO_PYTHON = NoThanks
 endif
 
+ifndef PAGER_ENV
+PAGER_ENV = LESS=-FRSX LV=-c
+endif
+
 QUIET_SUBDIR0  = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1  =
 
@@ -1598,6 +1610,11 @@ MAKE/%-string.h: MAKE/% script/mkcstring
$(subst -,_,$*) <$< >$@+ && \
mv $@+ $@
 
+MAKE/%-array.h: MAKE/% script/mkcarray
+   $(QUIET_GEN)$(SHELL_PATH) script/mkcarray \
+   $(subst -,_,$*) <$< >$@+ && \
+   mv $@+ $@
+
 MAKE/%.sh: MAKE/% script/mksh
$(QUIET_GEN)$(SHELL_PATH) script/mksh \
$(subst -,_,$*) <$< >$@+ && \
@@ -1726,6 +1743,9 @@ builtin/help.sp builtin/help.s builtin/help.o: 
EXTRA_CPPFLAGS = \
 
 version.sp version.s version.o: MAKE/VERSION-string.h MAKE/USER-AGENT-string.h
 
+$(eval $(call make-var,PAGER-ENV,pager environment,$(PAGER_ENV)))
+pager.sp pager.s pager.o: MAKE/PAGER-ENV-array.h
+
 $(BUILT_INS): git$X
$(QUIET_BUILT_IN)$(RM) $@ && \
ln $< $@ 2>/dev/null || \
@@ -1776,7 +1796,7 @@ $(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
 
-git-sh-setup: MAKE/DIFF.sh
+git-sh-setup: MAKE/DIFF.sh MAKE/PAGER-ENV.sh
 
 git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 627d289..be4a58a 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -158,10 +158,11 @@ git_pager() {
else
GIT_PAGER=cat
fi
-   : ${LESS=-FRSX}
-   : ${LV=-c}
-   export LESS LV
-
+   for vardef in $MAKE_PAGER_ENV
+   do
+   var=${vardef%%=*}
+   eval ": \${$vardef} && export $var"
+   done
eval "$GIT_PAGER" '"$@"'
 }
 
diff --git a/pager.c b/pager.c
index 0cc75a8..6db84c6 100644
--- a/pager.c
+++ b/pager.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "run-command.h"
 #include "sigchain.h"
+#include "argv-array.h"
+#include "MAKE/PAGER-ENV-array.h"
 
 #ifndef DEFAULT_PAGER
 #define DEFAULT_PAGER "less"
@@ -60,9 +62,26 @@ const char *git_pager(int stdout_is_tty)
return pager;
 }
 
+static void setup_pager_env(struct argv_array *env)
+{
+   int i;
+
+   for (i = 0; MAKE_PAGER_ENV[i]; i++) {
+   struct strbuf buf = STRBUF_INIT;
+   const char *p = MAKE_PAGER_ENV[i];
+   const char *eq = strchrnul(p, '=');
+
+   strbuf_add(&buf, p, eq - p);
+   if (!getenv(buf.buf))
+   argv_array_push(env, p);
+   strbuf_release(&buf);
+   }
+}
+
 void setup_pager(void)
 {
const char *pager = git_pager(isatty(1));
+   struct argv_array env = ARGV_ARRAY_INIT;
 
if (!pager || pager_in_use())
return;
@@ -80,17 +99,10 @@ void setup_pager(void)
pager_process.use_shell = 1;
pager_process.argv = pager_argv;
pager_process.in = -1;
-   if (!getenv("LESS") || !getenv("LV")) {
-   static const char *env[3];
-   int i = 0;
-
-   if (!getenv("LESS"))
-   env[i++] = "LESS=FRSX";
-   if (!getenv("LV"))
- 

[PATCH 11/13] Makefile: auto-build C strings from make variables

[Jeff King]

We already insert the values of some make variables into
files in MAKE/*. We can therefore build a simple pattern
rule for converting such a value into a C string in a header
file, which can then be included and used as normal.

The new system is demonstrated on version.c, where it can
replace the use of "-D" on the command-line. Note that we
still have to manually specify a dependency in the
Makefile. In an ideal world, we would auto-detect the header
dependency; however, there are two holdups:

  1. We cannot rely on having automatic header dependency
 generation on all platforms.

  2. Even when we do generate the dependencies, we rely on
 being able to compile the file once, which means our
 system cannot handle generated headers without a manual
 dependency to bootstrap it.

Signed-off-by: Jeff King 
---
This is a technique that I have used in other projects with great
success, as the make variable dependencies are represented as file
dependencies (which is the only type of dependency make knows about).

_But_ it ends up a lot less nice here because of the way we do
auto-dependencies. I'm totally open to revamping the way we handle our
header dependencies, but I didn't do that in this series.

 Makefile | 11 +++
 script/mkcstring | 18 ++
 version.c|  6 --
 3 files changed, 29 insertions(+), 6 deletions(-)
 create mode 100644 script/mkcstring

diff --git a/Makefile b/Makefile
index cd07a70..203171d 100644
--- a/Makefile
+++ b/Makefile
@@ -1593,6 +1593,11 @@ MAKE/$1: FORCE
fi
 endef
 
+MAKE/%-string.h: MAKE/% script/mkcstring
+   $(QUIET_GEN)$(SHELL_PATH) script/mkcstring \
+   $(subst -,_,$*) <$< >$@+ && \
+   mv $@+ $@
+
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -1614,6 +1619,7 @@ BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
 endif
 
 $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
+$(eval $(call make-var,VERSION,version,$(GIT_VERSION)))
 
 ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
@@ -1713,10 +1719,7 @@ builtin/help.sp builtin/help.s builtin/help.o: 
EXTRA_CPPFLAGS = \
-DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
-DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
-version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
-version.sp version.s version.o: EXTRA_CPPFLAGS = \
-   -DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
-   -DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
+version.sp version.s version.o: MAKE/VERSION-string.h MAKE/USER-AGENT-string.h
 
 $(BUILT_INS): git$X
$(QUIET_BUILT_IN)$(RM) $@ && \
diff --git a/script/mkcstring b/script/mkcstring
new file mode 100644
index 000..c01f430
--- /dev/null
+++ b/script/mkcstring
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+name=$1; shift
+
+c_quote() {
+   sed 's/\\//g; s/"/\\"/'
+}
+
+cat <<-EOF
+#ifndef MAKE_${name}_H
+#define MAKE_${name}_H
+
+/* Auto-generated by mkcstring */
+
+#define MAKE_${name} "$(c_quote)"
+
+#endif /* MAKE_${name}_H */
+EOF
diff --git a/version.c b/version.c
index 6106a80..f68a93b 100644
--- a/version.c
+++ b/version.c
@@ -1,8 +1,10 @@
 #include "git-compat-util.h"
 #include "version.h"
 #include "strbuf.h"
+#include "MAKE/USER-AGENT-string.h"
+#include "MAKE/VERSION-string.h"
 
-const char git_version_string[] = GIT_VERSION;
+const char git_version_string[] = MAKE_VERSION;
 
 const char *git_user_agent(void)
 {
@@ -11,7 +13,7 @@ const char *git_user_agent(void)
if (!agent) {
agent = getenv("GIT_USER_AGENT");
if (!agent)
-   agent = GIT_USER_AGENT;
+   agent = MAKE_USER_AGENT;
}
 
return agent;
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 01/13] Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS

[Jeff King]

This variable is used only by shell scripts, not by C
programs. It was originally included in GIT-CFLAGS as part
of ad17ea7 (add a Makefile switch to avoid gettext
translation in shell scripts, 2012-01-23), in an attempt to
trigger rebuilding when the variable changed.

However, shell scripts do not respect GIT-CFLAGS. Later,
e4dd89a (Makefile: update scripts when build-time parameters
change, 2012-06-20) introduced a separate GIT-SCRIPT-DEFINES
to accomplish this, which also included USE_GETTEXT_SCHEME.
We can drop the redundant and useless mention in GIT-CFLAGS.

Signed-off-by: Jeff King 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index dddaf4f..3cd4a92 100644
--- a/Makefile
+++ b/Makefile
@@ -2164,7 +2164,7 @@ GIT-PREFIX: FORCE
echo "$$FLAGS" >GIT-PREFIX; \
fi
 
-TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS)):$(USE_GETTEXT_SCHEME)
+TRACK_CFLAGS = $(CC):$(subst ','\'',$(ALL_CFLAGS))
 
 GIT-CFLAGS: FORCE
@FLAGS='$(TRACK_CFLAGS)'; \
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 03/13] Makefile: introduce make-var helper function

[Jeff King]

It's a common pattern in our Makefile to echo some make
variables into a file, but only if they are different from a
previous run. This sentinel file can then be used as a
dependency to trigger rebuilds when the make variable
changes.

The code to do this is a bit ugly and repetetive; let's
factor it out into a reusable function.

Note that this relies on the "call" and "eval" functions of
GNU make. We previously avoided using "call", as explained
in 39c015c (Fixes for ancient versions of GNU make,
2006-02-18). However, it has been 8 years since then, so
perhaps its use is acceptable now.

The "call" function dates back to GNU make 3.77.90
(1997-07-21). The "eval" function dates back to 3.80
(2002-07-08).

If it's still a problem to use these functions, we can
do similar meta-programming with something like:

include magic.mak
magic.mak:
./generate-magic-rules >$@+
mv $@+ $@

where the rules are generated by a shell (or other) script.

Signed-off-by: Jeff King 
---
 Makefile | 105 ---
 1 file changed, 40 insertions(+), 65 deletions(-)

diff --git a/Makefile b/Makefile
index 1f3e5d9..50bf252 100644
--- a/Makefile
+++ b/Makefile
@@ -1561,6 +1561,20 @@ ifneq ("$(PROFILE)","")
 endif
 endif
 
+# usage: $(eval $(call make-var,FN,DESC,CONTENTS))
+#
+# Create a rule to write $CONTENTS (which should come from a make variable)
+# to GIT-$FN, but only if not already there. This can be used to create a
+# dependency on a Makefile variable. Prints $DESC to the user.
+define make-var
+GIT-$1: FORCE
+   @VALUE='$$(subst ','\'',$3)'; \
+   if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
+   echo >&2 "* new $2"; \
+   echo "VALUE" >$$@; \
+   fi
+endef
+
 # Shell quote (do not use $(call) to accommodate ancient setups);
 
 SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
@@ -1615,13 +1629,9 @@ SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
 BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
 endif
 
-GIT_USER_AGENT_SQ = $(subst ','\'',$(GIT_USER_AGENT))
 GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
 GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
-GIT-USER-AGENT: FORCE
-   @if test x'$(GIT_USER_AGENT_SQ)' != x"`cat GIT-USER-AGENT 
2>/dev/null`"; then \
-   echo '$(GIT_USER_AGENT_SQ)' >GIT-USER-AGENT; \
-   fi
+$(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
 
 ifdef DEFAULT_HELP_FORMAT
 BASIC_CFLAGS += -DDEFAULT_HELP_FORMAT='"$(DEFAULT_HELP_FORMAT)"'
@@ -1737,9 +1747,17 @@ common-cmds.h: ./generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
$(QUIET_GEN)./generate-cmdlist.sh > $@+ && mv $@+ $@
 
-SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
-   $(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
-   $(gitwebdir_SQ):$(PERL_PATH_SQ)
+$(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
+   :$(SHELL_PATH)\
+   :$(DIFF)\
+   :$(GIT_VERSION)\
+   :$(localedir)\
+   :$(NO_CURL)\
+   :$(USE_GETTEXT_SCHEME)\
+   :$(SANE_TOOL_PATH)\
+   :$(gitwebdir)\
+   :$(PERL_PATH)\
+))
 define cmd_munge_script
 $(RM) $@ $@+ && \
 sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
@@ -1754,14 +1772,6 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 $@.sh >$@+
 endef
 
-GIT-SCRIPT-DEFINES: FORCE
-   @FLAGS='$(SCRIPT_DEFINES)'; \
-   if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
-   echo >&2 "* new script parameters"; \
-   echo "$$FLAGS" >$@; \
-fi
-
-
 $(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
chmod +x $@+ && \
@@ -1789,7 +1799,10 @@ perl/PM.stamp: FORCE
 perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' 
prefix='$(prefix_SQ)' $(@F)
 
-PERL_DEFINES = $(PERL_PATH_SQ):$(PERLLIB_EXTRA_SQ)
+$(eval $(call make-var,PERL-DEFINES,perl-specific parameters,\
+   :$(PERL_PATH)\
+   :$(PERLLIB_EXTRA)\
+))
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl perl/perl.mak GIT-PERL-DEFINES 
GIT-VERSION-FILE
$(QUIET_GEN)$(RM) $@ $@+ && \
INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
instlibdir` && \
@@ -1807,14 +1820,6 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl 
perl/perl.mak GIT-PERL-DEFINES G
chmod +x $@+ && \
mv $@+ $@
 
-GIT-PERL-DEFINES: FORCE
-   @FLAGS='$(PERL_DEFINES)'; \
-   if test x"$$FLAGS" != x"`cat $@ 2>/dev/null`" ; then \
-   echo >&2 "* new perl-specific parameters"; \
-   echo "$$FLAGS" >$@; \
-   fi
-
-
 .PHONY: gitweb
 gitweb:
$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
@@ -1835,6 +1840,7 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)) git-instaweb: %

[PATCH 10/13] Makefile: drop *_SQ variables

[Jeff King]

Now that all uses of the SQ variables have been dropped, we
can stop setting the variables.

Signed-off-by: Jeff King 
---
 Makefile | 28 
 1 file changed, 28 deletions(-)

diff --git a/Makefile b/Makefile
index b1c3fb4..cd07a70 100644
--- a/Makefile
+++ b/Makefile
@@ -1039,7 +1039,6 @@ endif
 endif
 
 ifdef SANE_TOOL_PATH
-SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH))
 BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(call 
sqi,$(SANE_TOOL_PATH)|'
 PATH := $(SANE_TOOL_PATH):${PATH}
 else
@@ -1594,31 +1593,6 @@ MAKE/$1: FORCE
fi
 endef
 
-# Shell quote (do not use $(call) to accommodate ancient setups);
-
-SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
-ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
-ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
-
-DESTDIR_SQ = $(subst ','\'',$(DESTDIR))
-bindir_SQ = $(subst ','\'',$(bindir))
-bindir_relative_SQ = $(subst ','\'',$(bindir_relative))
-mandir_relative_SQ = $(subst ','\'',$(mandir_relative))
-infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
-localedir_SQ = $(subst ','\'',$(localedir))
-gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
-template_dir_SQ = $(subst ','\'',$(template_dir))
-htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
-prefix_SQ = $(subst ','\'',$(prefix))
-gitwebdir_SQ = $(subst ','\'',$(gitwebdir))
-
-SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
-PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))
-PYTHON_PATH_SQ = $(subst ','\'',$(PYTHON_PATH))
-TCLTK_PATH_SQ = $(subst ','\'',$(TCLTK_PATH))
-DIFF_SQ = $(subst ','\'',$(DIFF))
-PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
-
 LIBS = $(GITLIBS) $(EXTLIBS)
 
 BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -2302,7 +2276,6 @@ gitexec_instdir = $(gitexecdir)
 else
 gitexec_instdir = $(prefix)/$(gitexecdir)
 endif
-gitexec_instdir_SQ = $(subst ','\'',$(gitexec_instdir))
 export gitexec_instdir
 
 ifneq ($(filter /%,$(firstword $(mergetoolsdir))),)
@@ -2310,7 +2283,6 @@ mergetools_instdir = $(mergetoolsdir)
 else
 mergetools_instdir = $(prefix)/$(mergetoolsdir)
 endif
-mergetools_instdir_SQ = $(subst ','\'',$(mergetools_instdir))
 
 install_bindir_programs := $(patsubst %,%$X,$(BINDIR_PROGRAMS_NEED_X)) 
$(BINDIR_PROGRAMS_NO_X)
 
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 05/13] Makefile: prefer printf to echo for GIT-*

[Jeff King]

When we write Makefile variables to a sentinel file, we use
"echo" to do so. Since we are writing arbitrary data which
may contain backslash escapes (particularly with file paths
on Windows), echo may or may not expand those escapes,
depending on which shell is in use.

During the next run of "make", we check the sentinel file to
see if it is different than the Makefile variable. If
escapes were expanded, then we will erroneously think it
changed and trigger a rebuild. You can see this easily by
running:

  make prefix='foo\bar'

multiple times; it will re-build some files repeatedly.

Signed-off-by: Jeff King 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index b06d5ec..60dc53b 100644
--- a/Makefile
+++ b/Makefile
@@ -1571,7 +1571,7 @@ GIT-$1: FORCE
@VALUE='$$(subst ','\'',$3)'; \
if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
echo >&2 "* new $2"; \
-   echo "VALUE" >$$@+ && \
+   printf '%s\n' "VALUE" >$$@+ && \
mv $$@+ $$@; \
fi
 endef
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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] repack.c: rename and unlink pack file if it exists

[Torsten Bögershausen]

On 2014-02-05 02.16, Jeff King wrote:
> On Tue, Feb 04, 2014 at 03:40:15PM -0800, Junio C Hamano wrote:
> 
>>  * Somehow this came to my private mailbox without Cc to list, so I
>>am forwarding it.
>>
>>I think with 1190a1ac (pack-objects: name pack files after
>>trailer hash, 2013-12-05), repacking the same set of objects may
>>have less chance of producing colliding names, especially if you
>>are on a box with more than one core, but it still would be a
>>good idea to get this part right in the upcoming release.
> 
> Actually, since 1190a1ac, if you have repacked and gotten the same pack
> name, then you do not have to do any rename dance at all; you can throw
> away what you just generated because you know that it is byte-for-byte
> identical.
> 
> You could collide with a pack created by an older version of git that
> used the original scheme, but that is quite unlikely (on the order of
> 2^-160).
> 
> -Peff
OK, I messed up the email so it went only to Junios mailbox. Sorry for 
confusion.

Jochen, it could be good if you could test this version of the patch
(I couldn't reproduce the problem here)

/Torsten


--
To unsubscribe from this list: send the line "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 08/13] Makefile: introduce sq function for shell-quoting

[Jeff King]

Since we have recently abolished the prohibition on $(call)
in our Makefile, we can use it to factor out the repeated
shell-quoting we do. There are two upsides:

  1. It is much more readable than inline calls to
 $(subst ','\'').

  2. It is short enough that we can do away with the _SQ
 variants of many variables (note that we do not do this
 just yet, as there is a little more cleanup needed
 first).

Signed-off-by: Jeff King 
---
This is the one I am most on the fence about. I think (2) is nice. And
for (1), we do end up more readable in some instances (e.g., the
horrible inline subst calls in generating BUILD-OPTIONS go away). But
many instances are not really any more readable (e.g., see the first
hunk below).

Two things come to mind:

  1. If we really prefer reading $(FOO_SQ) but don't want to manually
 write each one, we could have a simple script that reads the
 Makefile and produces an _SQ variant of every variable, whether we
 need it or not.

  2. The later parts of the series introduce a new way of getting
 values into programs that does not involve the command-line. So in
 theory many of these quoted uses of variables would just go away in
 the long run (if we choose to pursue that direction).

 Makefile | 120 ++-
 1 file changed, 64 insertions(+), 56 deletions(-)

diff --git a/Makefile b/Makefile
index e12039f..868872f 100644
--- a/Makefile
+++ b/Makefile
@@ -506,11 +506,11 @@ build-python-script: $(SCRIPT_PYTHON_GEN)
 
 .PHONY: install-perl-script install-sh-script install-python-script
 install-sh-script: $(SCRIPT_SH_INS)
-   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+   $(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 install-perl-script: $(SCRIPT_PERL_INS)
-   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+   $(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 install-python-script: $(SCRIPT_PYTHON_INS)
-   $(INSTALL) $^ '$(DESTDIR_SQ)$(gitexec_instdir_SQ)'
+   $(INSTALL) $^ $(call sq,$(DESTDIR)$(gitexec_instdir))
 
 .PHONY: clean-perl-script clean-sh-script clean-python-script
 clean-sh-script:
@@ -1040,7 +1040,7 @@ endif
 
 ifdef SANE_TOOL_PATH
 SANE_TOOL_PATH_SQ = $(subst ','\'',$(SANE_TOOL_PATH))
-BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix 
$(SANE_TOOL_PATH_SQ)|'
+BROKEN_PATH_FIX = 's|^\# @@BROKEN_PATH_FIX@@$$|git_broken_path_fix $(call 
sqi,$(SANE_TOOL_PATH)|'
 PATH := $(SANE_TOOL_PATH):${PATH}
 else
 BROKEN_PATH_FIX = '/^\# @@BROKEN_PATH_FIX@@$$/d'
@@ -1561,6 +1561,13 @@ ifneq ("$(PROFILE)","")
 endif
 endif
 
+# usage: $(call sq,CONTENTS)
+#
+# Quote the value as appropriate for the shell. Use "sqi" if you are
+# already "i"nside single-quotes.
+sqi = $(subst ','\'',$1)
+sq = '$(call sqi,$1)'
+
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
@@ -1568,7 +1575,7 @@ endif
 # dependency on a Makefile variable. Prints $DESC to the user.
 define make-var
 MAKE/$1: FORCE
-   @VALUE='$$(subst ','\'',$3)'; \
+   @VALUE=$$(call sq,$3); \
if ! test -e $$@ || test x"VALUE" != x"`cat $$@`"; then \
echo >&2 "* new $2"; \
printf '%s\n' "VALUE" >$$@+ && \
@@ -1603,7 +1610,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 
 LIBS = $(GITLIBS) $(EXTLIBS)
 
-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
+BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
$(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
@@ -1665,13 +1672,13 @@ endif
 
 all::
 ifndef NO_TCLTK
-   $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) 
gitexecdir='$(gitexec_instdir_SQ)' all
+   $(QUIET_SUBDIR0)git-gui $(QUIET_SUBDIR1) gitexecdir=$(call 
sq,$(gitexec_instdir)) all
$(QUIET_SUBDIR0)gitk-git $(QUIET_SUBDIR1) all
 endif
 ifndef NO_PERL
-   $(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' 
prefix='$(prefix_SQ)' localedir='$(localedir_SQ)' all
+   $(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH=$(call sq,$(PERL_PATH)) 
prefix=$(call sq,$(prefix)) localedir=$(call sq,$(localedir)) all
 endif
-   $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) 
SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
+   $(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) SHELL_PATH=$(call 
sq,$(SHELL_PATH)) PERL_PATH=$(call sq,$(PERL_PATH))
 
 please_set_SHELL_PATH_to_a_more_modern_shell:
@$$(:)
@@ -1761,15 +1768,15 @@ $(eval $(call make-var,SCRIPT-DEFINES,script 
parameters,\
 ))
 define cmd_munge_script
 $(RM) $@ $@+ && \
-sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
--e 's|@SHELL_PATH@|$(SHELL_PATH_SQ)|' \
--e 's|@@DIFF@@|$(DIFF_SQ)|' \
--e 's|@@LOCALEDIR@@|$(localedir_SQ)|g' \
+sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
+-e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
+-e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
+-e 's|@@LOCALEDIR@@|$(call sqi,$(lo

[PATCH 09/13] Makefile: add c-quote helper function

[Jeff King]

We have to c-quote strings coming from Makefile variables
when we pass them to the compiler via "-D". Now that we can
use $(call) in our Makefile, we can factor out the quoting
to make things easier to read. We can also apply it more
consistently, as there were many spots that should have been
C-quoting but did not. For example:

  make prefix='foo\bar'

would produce an exec_cmd.o with a broken prefix.

Signed-off-by: Jeff King 
---
 Makefile | 58 +-
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/Makefile b/Makefile
index 868872f..b1c3fb4 100644
--- a/Makefile
+++ b/Makefile
@@ -1568,6 +1568,17 @@ endif
 sqi = $(subst ','\'',$1)
 sq = '$(call sqi,$1)'
 
+# usage: $(call cq,CONTENTS)
+#
+# Quote the value as appropriate for a C string literal.
+cq = "$(subst ",\",$(subst \,\\,$1))"
+
+# usage: $(call scq,CONTENTS)
+#
+# Quote the value as C string inside a shell string. Good for passing strings
+# on the command line via "-DFOO=$(call # scq,$(FOO))".
+scq = $(call sq,$(call cq,$1))
+
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
@@ -1617,28 +1628,17 @@ LIB_OBJS += $(COMPAT_OBJS)
 # Quote for C
 
 ifdef DEFAULT_EDITOR
-DEFAULT_EDITOR_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_EDITOR)))"
-DEFAULT_EDITOR_CQ_SQ = $(subst ','\'',$(DEFAULT_EDITOR_CQ))
-
-BASIC_CFLAGS += -DDEFAULT_EDITOR='$(DEFAULT_EDITOR_CQ_SQ)'
+BASIC_CFLAGS += -DDEFAULT_EDITOR=$(call scq,$(DEFAULT_EDITOR))
 endif
 
 ifdef DEFAULT_PAGER
-DEFAULT_PAGER_CQ = "$(subst ",\",$(subst \,\\,$(DEFAULT_PAGER)))"
-DEFAULT_PAGER_CQ_SQ = $(subst ','\'',$(DEFAULT_PAGER_CQ))
-
-BASIC_CFLAGS += -DDEFAULT_PAGER='$(DEFAULT_PAGER_CQ_SQ)'
+BASIC_CFLAGS += -DDEFAULT_PAGER=$(call scq,$(DEFAULT_PAGER))
 endif
 
 ifdef SHELL_PATH
-SHELL_PATH_CQ = "$(subst ",\",$(subst \,\\,$(SHELL_PATH)))"
-SHELL_PATH_CQ_SQ = $(subst ','\'',$(SHELL_PATH_CQ))
-
-BASIC_CFLAGS += -DSHELL_PATH='$(SHELL_PATH_CQ_SQ)'
+BASIC_CFLAGS += -DSHELL_PATH=$(call scq,$(SHELL_PATH))
 endif
 
-GIT_USER_AGENT_CQ = "$(subst ",\",$(subst \,\\,$(GIT_USER_AGENT)))"
-GIT_USER_AGENT_CQ_SQ = $(subst ','\'',$(GIT_USER_AGENT_CQ))
 $(eval $(call make-var,USER-AGENT,user agent string,$(GIT_USER_AGENT)))
 
 ifdef DEFAULT_HELP_FORMAT
@@ -1723,9 +1723,9 @@ strip: $(PROGRAMS) git$X
 
 git.sp git.s git.o: MAKE/PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
-   '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
-   '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
-   '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
+   -DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
+   -DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
+   -DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
 git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
@@ -1735,14 +1735,14 @@ help.sp help.s help.o: common-cmds.h
 
 builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
-   '-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
-   '-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
-   '-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
+   -DGIT_HTML_PATH=$(call scq,$(htmldir_relative)) \
+   -DGIT_MAN_PATH=$(call scq,$(mandir_relative)) \
+   -DGIT_INFO_PATH=$(call scq,$(infodir_relative))
 
 version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
-   '-DGIT_VERSION="$(GIT_VERSION)"' \
-   '-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
+   -DGIT_VERSION=$(call scq,$(GIT_VERSION)) \
+   -DGIT_USER_AGENT=$(call scq,$(GIT_USER_AGENT))
 
 $(BUILT_INS): git$X
$(QUIET_BUILT_IN)$(RM) $@ && \
@@ -2020,25 +2020,25 @@ endif
 
 exec_cmd.sp exec_cmd.s exec_cmd.o: MAKE/PREFIX
 exec_cmd.sp exec_cmd.s exec_cmd.o: EXTRA_CPPFLAGS = \
-   '-DGIT_EXEC_PATH="$(gitexecdir_SQ)"' \
-   '-DBINDIR="$(bindir_relative_SQ)"' \
-   '-DPREFIX="$(prefix_SQ)"'
+   -DGIT_EXEC_PATH=$(call scq,$(gitexecdir)) \
+   -DBINDIR=$(call scq,$(bindir_relative)) \
+   -DPREFIX=$(call scq,$(prefix))
 
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: MAKE/PREFIX
 builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
-   -DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
+   -DDEFAULT_GIT_TEMPLATE_DIR=$(call scq,$(template_dir))
 
 config.sp config.s config.o: MAKE/PREFIX
 config.sp config.s config.o: EXTRA_CPPFLAGS = \
-   -DETC_GITCONFIG='"$(ETC_GITCONFIG_SQ)"'
+   -DETC_GITCONFIG=$(call scq,$(ETC_GITCONFIG))
 
 attr.sp attr.s attr.o: MAKE/PREFIX
 attr.sp attr.s attr.o: EXTRA_CPPFLAGS = \
-   -DETC_GITATTRIBUTES='"$(ETC_GITATTRIBUTES_SQ)"'
+   -DETC_GITATTRIBUTES=$(call scq,$(ETC_GITATTRIBUTES))
 
 gettext.sp gettext.s gettext.o: MAKE/PREFIX
 gettext.sp gettext.s gettext.o: EXTRA_CPPFLAGS = \
-   -DGIT_LOCALE_PATH='"$(localedir_SQ)"'
+ 

[PATCH 06/13] Makefile: store GIT-* sentinel files in MAKE/

[Jeff King]

The Makefile creates files like GIT-CFLAGS to keep track of
the make variables used in the last build. We have a number
of these files now, all starting with GIT-*. Let's move them
into their own subdirectory, which has two advantages:

  1. It's less clutter in the main directory.

  2. Each file needs to be marked separately in .gitignore.
 With a subdirectory, we can easily mark them all with a
 wildcard (we cannot do a wildcard for GIT-* because
 of precious files like GIT-VERSION-GEN).

This patch moves all of the generated GIT-* files into
MAKE/*, with one exception: GIT-VERSION-FILE. This could be
moved along with the rest, but there is a reasonable chance
that there are some out-of-tree scripts that may peek at it
(whereas things like GIT-CFLAGS are purely internal, and we
update all internal references).

Signed-off-by: Jeff King 
---
I'm not married to the name "MAKE", but I do think the separate
directory is a win for simplicity and avoiding repetition (as you can
see in the diffstat). Other suggestions welcome.

 .gitignore|  8 ---
 MAKE/.gitignore   |  1 +
 Makefile  | 66 +--
 perl/Makefile | 10 
 t/perf/run|  2 +-
 t/test-lib.sh |  2 +-
 t/valgrind/analyze.sh |  4 ++--
 7 files changed, 42 insertions(+), 51 deletions(-)
 create mode 100644 MAKE/.gitignore

diff --git a/.gitignore b/.gitignore
index b5f9def..586a067 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,11 +1,3 @@
-/GIT-BUILD-OPTIONS
-/GIT-CFLAGS
-/GIT-LDFLAGS
-/GIT-PREFIX
-/GIT-PERL-DEFINES
-/GIT-PYTHON-VARS
-/GIT-SCRIPT-DEFINES
-/GIT-USER-AGENT
 /GIT-VERSION-FILE
 /bin-wrappers/
 /git
diff --git a/MAKE/.gitignore b/MAKE/.gitignore
new file mode 100644
index 000..72e8ffc
--- /dev/null
+++ b/MAKE/.gitignore
@@ -0,0 +1 @@
+*
diff --git a/Makefile b/Makefile
index 60dc53b..7fecdf1 100644
--- a/Makefile
+++ b/Makefile
@@ -1564,10 +1564,10 @@ endif
 # usage: $(eval $(call make-var,FN,DESC,CONTENTS))
 #
 # Create a rule to write $CONTENTS (which should come from a make variable)
-# to GIT-$FN, but only if not already there. This can be used to create a
+# to MAKE/$FN, but only if not already there. This can be used to create a
 # dependency on a Makefile variable. Prints $DESC to the user.
 define make-var
-GIT-$1: FORCE
+MAKE/$1: FORCE
@VALUE='$$(subst ','\'',$3)'; \
if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
echo >&2 "* new $2"; \
@@ -1658,7 +1658,7 @@ all:: profile-clean
 endif
 endif
 
-all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) 
GIT-BUILD-OPTIONS
+all:: $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS) 
MAKE/BUILD-OPTIONS
 ifneq (,$X)
$(QUIET_BUILT_IN)$(foreach p,$(patsubst %$X,%,$(filter 
%$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), test -d '$p' -o '$p' -ef '$p$X' || 
$(RM) '$p';)
 endif
@@ -1714,25 +1714,25 @@ strip: $(PROGRAMS) git$X
 #   dependencies here will not need to change if the force-build
 #   details change some day.
 
-git.sp git.s git.o: GIT-PREFIX
+git.sp git.s git.o: MAKE/PREFIX
 git.sp git.s git.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
+git$X: git.o MAKE/LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
 
 help.sp help.s help.o: common-cmds.h
 
-builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
+builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h MAKE/PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
+version.sp version.s version.o: GIT-VERSION-FILE MAKE/USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
'-DGIT_VERSION="$(GIT_VERSION)"' \
'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)'
@@ -1773,12 +1773,12 @@ sed -e '1s|#!.*/sh|#!$(SHELL_PATH_SQ)|' \
 $@.sh >$@+
 endef
 
-$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh GIT-SCRIPT-DEFINES
+$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
chmod +x $@+ && \
mv $@+ $@
 
-$(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
+$(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
 
@@ -1797,14 +1797,14 @@ perl/PM.stamp: FORCE
{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; } && \
$(RM) $@+
 
-perl/perl.mak: GIT-CFLAGS GIT-PREFIX perl/Makefile perl/Makefile.PL
+perl/perl.mak: MAKE/CFLAGS MAKE/PREFIX perl/Makefile perl/Makefile.PL
$(QUIET_S

[PATCH 07/13] Makefile: always create files via make-var

[Jeff King]

If we are trying to store an empty sentinel value for a
make-var, we would consider a missing file the same as an
empty variable. E.g., repeatedly running:

  make GIT_USER_AGENT=

will not create MAKE/USER-AGENT at all if it does not exist,
and subsequent make invocations will force a rebuild. This
is not generally a problem in practice, since most of the
files always have some boilerplate (even LDFLAGS, because it
is formed with "+=", will have a stray space in it). But
this does fix the rare case, and future-proofs us as we add
more similar variables.

Signed-off-by: Jeff King 
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 7fecdf1..e12039f 100644
--- a/Makefile
+++ b/Makefile
@@ -1569,7 +1569,7 @@ endif
 define make-var
 MAKE/$1: FORCE
@VALUE='$$(subst ','\'',$3)'; \
-   if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
+   if ! test -e $$@ || test x"VALUE" != x"`cat $$@`"; then \
echo >&2 "* new $2"; \
printf '%s\n' "VALUE" >$$@+ && \
mv $$@+ $$@; \
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 04/13] Makefile: use tempfile/mv strategy for GIT-*

[Jeff King]

We create GIT-CFLAGS and related files by echoing into a
shell redirection. It's possible for an error to cause the
file to end up empty or truncated. In theory, this could
cause us to later make an incorrect comparison of the file
contents to a Makefile variable, and avoid rebuilding some
dependencies.

In practice, this is very unlikely to happen, as the
followup run would have to have changed the variable to
match the truncation exactly. However, it is good hygiene to
use our usual tempfile + mv trick to make sure that the file
is created atomically.

Signed-off-by: Jeff King 
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 50bf252..b06d5ec 100644
--- a/Makefile
+++ b/Makefile
@@ -1571,7 +1571,8 @@ GIT-$1: FORCE
@VALUE='$$(subst ','\'',$3)'; \
if test x"VALUE" != x"`cat $$@ 2>/dev/null`"; then \
echo >&2 "* new $2"; \
-   echo "VALUE" >$$@; \
+   echo "VALUE" >$$@+ && \
+   mv $$@+ $$@; \
fi
 endef
 
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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 02/13] Makefile: fix git-instaweb dependency on gitweb

[Jeff King]

We build git-instaweb if NO_PERL is not defined, and it in
turns depends on "gitweb", which causes us to descend into
the gitweb subdir and recursively invoke make. But because
gitweb is marked as .PHONY, this forces a rebuild of
git-instaweb every time we run make.

We can drop the dependency of git-instaweb on gitweb.
However, that means we do not recurse into "gitweb" at all,
so we have to add back in the dependency to "all".

Note that the other subdirectories (like git-gui/ or perl/)
are handled directly as build commands of "all", and we
could simply do the same thing here. However, this patch
keeps "gitweb" as a target so that "make gitweb" continues
to work, in case anybody is using that.

Signed-off-by: Jeff King 
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 3cd4a92..1f3e5d9 100644
--- a/Makefile
+++ b/Makefile
@@ -1818,8 +1818,9 @@ GIT-PERL-DEFINES: FORCE
 .PHONY: gitweb
 gitweb:
$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
+all:: gitweb
 
-git-instaweb: git-instaweb.sh gitweb GIT-SCRIPT-DEFINES
+git-instaweb: git-instaweb.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
chmod +x $@+ && \
mv $@+ $@
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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/RFC 0/13] makefile refactoring

[Jeff King]

This started with the desire to move the setting of the LESS and LV
environment variables into a Makefile knob. But there's a fair bit of
infrastructure involved in that, and this is an attempt to factor out
some of that infrastructure to be more easily reusable. And if we like
the approach, we can move more build-time config in this direction.

There are a couple of things going on here, but the main ideas are:

  1. Try to get build-time data into files as much as possible, because
 the one thing make understands is dependencies between files. Right
 now we rely on sentinel files like GIT-CFLAGS, which have two
 downsides:

   a. It is easy to miss a dependency that should be in a sentinel
  file, leading to failure to rebuild when we should.

   b. Because they are so cumbersome to use, we tend to put a lot of
  items into a small number of sentinel files, leading to
  unnecessary rebuilds (e.g., turning on XDL_FAST_HASH
  recompiles _everything_, even though only one C file cares
  about it).

  2. Some light meta-programming to avoid repeating ourselves and try to
 make a few things more readable. I've done this here with $(call)
 and $(eval), which are basically the only way to do this in GNU
 make (and here we are relying heavily on GNU make, but as far as I
 know, nobody has had a huge problem with that in the past).

 Frankly, some of it is kind of ugly. And there's some potential for
 portability/version problems, just because we're not using more
 advanced features. If we don't like that approach, an alternative
 is to generate snippets of Makefile in a separate script and
 include them (like we do already for GIT-VERSION-FILE). That would
 let us write in whatever language we want, and avoid portability
 problems. The downside is that it may be less obvious to a reader
 not familiar with the system (e.g., you cannot necessarily grep for
 all the rules in Makefile, though that is already somewhat the case
 with pattern rules).

The two potential criticisms I expect are:

  1. Portability issues with $(call), as mentioned above. We avoided
 this in 2006, but it may be sufficiently available now. See patch 3
 for exact numbers/versions.

  2. Some people may simply find it ugly or too confusing for the
 benefit. Hence the RFC. :)

While I tried to polish these patches enough to be applied, please take
this mostly as an RFC on the ideas and direction. There are multiple
alternatives to implement some of these things, and I mainly want to see
if people think this sort of make meta-programming is a good idea.  The
patches are:

  [01/13]: Makefile: drop USE_GETTEXT_SCHEME from GIT-CFLAGS
  [02/13]: Makefile: fix git-instaweb dependency on gitweb

These first two are cleanups I noticed in the area, and can be
applied regardless of the rest.

  [03/13]: Makefile: introduce make-var helper function
  [04/13]: Makefile: use tempfile/mv strategy for GIT-*
  [05/13]: Makefile: prefer printf to echo for GIT-*
  [06/13]: Makefile: store GIT-* sentinel files in MAKE/
  [07/13]: Makefile: always create files via make-var

These ones factor out and improve the GIT-* file handling. Even if
we decide not to go this route, patches 4, 5, and 7 are improvements
that could apply to the current code. I didn't float them to the top
of the series because it would involve making the same change in
several different spots. If we decide not to apply this series, I
can re-roll them as appropriate.

  [08/13]: Makefile: introduce sq function for shell-quoting
  [09/13]: Makefile: add c-quote helper function
  [10/13]: Makefile: drop *_SQ variables

If we accept that we can use $(call), these are further readability
cleanups we can do. They are technically optional, though, with
respect to the rest of the series.

  [11/13]: Makefile: auto-build C strings from make variables
  [12/13]: Makefile: teach scripts to include make variables

These ones lay the groundwork for easily getting make variables into
shell scripts and C programs.

  [13/13]: move LESS/LV pager environment to Makefile

And this one is the point of the series, which is fairly
straightforward because of the earlier groundwork.

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

Re: [PATCH 4/4] revision: convert to using diff_tree_sha1()

[Jeff King]

On Wed, Feb 05, 2014 at 08:57:12PM +0400, Kirill Smelkov wrote:

> Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
> could just call it without manually reading trees into tree_desc and
> duplicating code.
> 
> Besides, that
> 
>   if (!tree)
>   return 0;
> 
> looked suspect - we were saying an invalid tree != empty tree, but maybe it is
> better to just say the tree is invalid here, which is what diff_tree_sha1()
> does for such case.

I think that is sensible. The assertion that "invalid != empty" is
probably sane, because we handle the empty tree as internal magic. But I
do not see any reason we should be hitting this code path regularly with
an invalid tree, short of repository corruption, so in practice I don't
think it matters.

This does introduce a die() where there was not one previously, and that
can make things harder to diagnose/debug in a corrupted repository. But
it looks like this is limited to the history-simplification code, and I
suspect that it is not commonly used in the case of corruption.

So I think the patch looks fine.

-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 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Jeff King]

On Wed, Feb 05, 2014 at 08:57:08PM +0400, Kirill Smelkov wrote:

> Kirill Smelkov (4):
>   tree-diff: allow diff_tree_sha1 to accept NULL sha1
>   tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1
> with old=NULL
>   line-log: convert to using diff_tree_sha1()
>   revision: convert to using diff_tree_sha1()
> 
>  line-log.c  | 26 ++
>  revision.c  | 12 +---
>  tree-diff.c | 27 +--
>  3 files changed, 8 insertions(+), 57 deletions(-)

Yay, I like the diffstat. All of the patches look good to me.

-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] userdiff: update Ada patterns

[Jeff King]

On Wed, Feb 05, 2014 at 09:17:47AM -0800, Junio C Hamano wrote:

> Adrian Johnson  writes:
> 
> >>> -  "|[0-9][-+0-9#_.eE]"
> >>> +  "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
> >> 
> >> This would match a lot wider than what I read you said you wanted to
> >> match in your previous message.  Does "-04##4_3_2Ee-9" count as a
> >> number, for example, or can we just ignore such syntactically
> >> incorrect sequence?
> >
> > Maybe I am misunderstanding the purpose of the word diff regexes. I
> > thought the purpose of the word regex is to split lines into words, not
> > determine what is syntactically correct.
> 
> I agree that the purpose is former---So you could have just said
> "the latter" ;-).
> 
> Any other nitpick, anybody?  Otherwise I'll queue this version.

No nitpick here, I had the same thought as Adrian while reading the
thread (and if somebody comes up with a real case where the output looks
bad, we can iterate on it).

-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 0/3] Add a function skip_prefix_if_present()

[Jeff King]

On Wed, Feb 05, 2014 at 07:55:09AM +0100, Michael Haggerty wrote:

> * René Scharfe submitted a patch to use a function parse_prefix()
>   (originally suggested by Peff) instead of Duy's suggested approach:
> 
>   http://article.gmane.org/gmane.comp.version-control.git/239569
> 
>   His patch appears to have been overlooked.
> 
> * Duy seemed to offer to rewrite his patch series, but I don't think
>   that it has happened yet.
> 
> And then the conversation was drowned by Christmas eggnog.
> 
> I don't have a strong feeling about (Duy's proposal plus my patches) vs.
> (René's parse_prefix() approach).  But I definitely *do* like the idea
> of getting rid of all those awkward magic numbers everywhere.

FWIW, after coming down off my eggnog bender, I think I still prefer
René's (my?) approach.

-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] userdiff: update Ada patterns

[Junio C Hamano]

Adrian Johnson  writes:

>>> -"|[0-9][-+0-9#_.eE]"
>>> +"|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>> 
>> This would match a lot wider than what I read you said you wanted to
>> match in your previous message.  Does "-04##4_3_2Ee-9" count as a
>> number, for example, or can we just ignore such syntactically
>> incorrect sequence?
>
> Maybe I am misunderstanding the purpose of the word diff regexes. I
> thought the purpose of the word regex is to split lines into words, not
> determine what is syntactically correct.

I agree that the purpose is former---So you could have just said
"the latter" ;-).

Any other nitpick, anybody?  Otherwise I'll queue this version.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] reset: support "--mixed --intent-to-add" mode

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Feb 04, 2014 at 02:25:25PM -0800, Junio C Hamano wrote:
>> Junio C Hamano  writes:
>> 
>> > While I do not have any problem with adding an optional "keep lost
>> > paths as intent-to-add entries" feature, I am not sure why this has
>> > to be so different from the usual add-cache-entry codepath.  The
>> > if/elseif chain you are touching inside this loop does:
>> >
>> >  - If the tree you are resetting to has something at the path
>> >(which is different from the current index, obviously), create
>> >a cache entry to represent that state from the tree and stuff
>> >it in the index;
>> >
>> >  - Otherwise, the tree you are resetting to does not have that
>> >path.  We used to say "remove it from the index", but now we have
>> >an option to instead add it as an intent-to-add entry.
>> >
>> > So, why doesn't the new codepath do exactly the same thing as the
>> > first branch of the if/else chain and call add_cache_entry but with
>> > a ce marked with CE_INTENT_TO_ADD?  That would parallel what happens
>> > in "git add -N" better, I would think, no?
>> 
>> In other words, something along this line, perhaps?
>
> 
>
> Yes. But you need something like this on top to actually set
> CE_INTENT_TO_ADD

Yes, indeed.  I wonder why your new test did not notice it, though
;-)


>
> -- 8< --
> diff --git a/read-cache.c b/read-cache.c
> index 325d193..87f1367 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -585,6 +585,7 @@ void mark_intent_to_add(struct cache_entry *ce)
>   if (write_sha1_file("", 0, blob_type, sha1))
>   die("cannot create an empty blob in the object database");
>   hashcpy(ce->sha1, sha1);
> + ce->ce_flags |= CE_INTENT_TO_ADD;
>  }
>  
>  int add_to_index(struct index_state *istate, const char *path, struct stat 
> *st, int flags)
> -- 8< --
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] line-log: convert to using diff_tree_sha1()

[Kirill Smelkov]

Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
could just call it without manually reading trees into tree_desc and
duplicating code.

Cc: Thomas Rast 
Signed-off-by: Kirill Smelkov 
---
 line-log.c | 26 ++
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/line-log.c b/line-log.c
index 717638b..1500101 100644
--- a/line-log.c
+++ b/line-log.c
@@ -766,16 +766,6 @@ void line_log_init(struct rev_info *rev, const char 
*prefix, struct string_list
}
 }
 
-static void load_tree_desc(struct tree_desc *desc, void **tree,
-  const unsigned char *sha1)
-{
-   unsigned long size;
-   *tree = read_object_with_reference(sha1, tree_type, &size, NULL);
-   if (!*tree)
-   die("Unable to read tree (%s)", sha1_to_hex(sha1));
-   init_tree_desc(desc, *tree, size);
-}
-
 static int count_parents(struct commit *commit)
 {
struct commit_list *parents = commit->parents;
@@ -842,18 +832,11 @@ static void queue_diffs(struct line_log_data *range,
struct diff_queue_struct *queue,
struct commit *commit, struct commit *parent)
 {
-   void *tree1 = NULL, *tree2 = NULL;
-   struct tree_desc desc1, desc2;
-
assert(commit);
-   load_tree_desc(&desc2, &tree2, commit->tree->object.sha1);
-   if (parent)
-   load_tree_desc(&desc1, &tree1, parent->tree->object.sha1);
-   else
-   init_tree_desc(&desc1, "", 0);
 
DIFF_QUEUE_CLEAR(&diff_queued_diff);
-   diff_tree(&desc1, &desc2, "", opt);
+   diff_tree_sha1(parent ? parent->tree->object.sha1 : NULL,
+   commit->tree->object.sha1, "", opt);
if (opt->detect_rename) {
filter_diffs_for_paths(range, 1);
if (diff_might_be_rename())
@@ -861,11 +844,6 @@ static void queue_diffs(struct line_log_data *range,
filter_diffs_for_paths(range, 0);
}
move_diff_queue(queue, &diff_queued_diff);
-
-   if (tree1)
-   free(tree1);
-   if (tree2)
-   free(tree2);
 }
 
 static char *get_nth_line(long line, unsigned long *ends, void *data)
-- 
1.9.rc1.181.g641f458

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

[PATCH 2/4] tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1 with old=NULL

[Kirill Smelkov]

Now since diff_tree_sha1 understands NULL for both old and new, we could
indicate an empty tree for root commit by providing just NULL for old
sha1.

Signed-off-by: Kirill Smelkov 
---
 tree-diff.c | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index f438478..6d82a3f 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -304,18 +304,5 @@ int diff_tree_sha1(const unsigned char *old, const 
unsigned char *new, const cha
 
 int diff_root_tree_sha1(const unsigned char *new, const char *base, struct 
diff_options *opt)
 {
-   int retval;
-   void *tree;
-   unsigned long size;
-   struct tree_desc empty, real;
-
-   tree = read_object_with_reference(new, tree_type, &size, NULL);
-   if (!tree)
-   die("unable to read root tree (%s)", sha1_to_hex(new));
-   init_tree_desc(&real, tree, size);
-
-   init_tree_desc(&empty, "", 0);
-   retval = diff_tree(&empty, &real, base, opt);
-   free(tree);
-   return retval;
+   return diff_tree_sha1(NULL, new, base, opt);
 }
-- 
1.9.rc1.181.g641f458

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

[PATCH 4/4] revision: convert to using diff_tree_sha1()

[Kirill Smelkov]

Since diff_tree_sha1() can now accept empty trees via NULL sha1, we
could just call it without manually reading trees into tree_desc and
duplicating code.

Besides, that

if (!tree)
return 0;

looked suspect - we were saying an invalid tree != empty tree, but maybe it is
better to just say the tree is invalid here, which is what diff_tree_sha1()
does for such case.

Signed-off-by: Kirill Smelkov 
---
 revision.c | 12 +---
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/revision.c b/revision.c
index 082dae6..bd027bc 100644
--- a/revision.c
+++ b/revision.c
@@ -497,24 +497,14 @@ static int rev_compare_tree(struct rev_info *revs,
 static int rev_same_tree_as_empty(struct rev_info *revs, struct commit *commit)
 {
int retval;
-   void *tree;
-   unsigned long size;
-   struct tree_desc empty, real;
struct tree *t1 = commit->tree;
 
if (!t1)
return 0;
 
-   tree = read_object_with_reference(t1->object.sha1, tree_type, &size, 
NULL);
-   if (!tree)
-   return 0;
-   init_tree_desc(&real, tree, size);
-   init_tree_desc(&empty, "", 0);
-
tree_difference = REV_TREE_SAME;
DIFF_OPT_CLR(&revs->pruning, HAS_CHANGES);
-   retval = diff_tree(&empty, &real, "", &revs->pruning);
-   free(tree);
+   retval = diff_tree_sha1(NULL, t1->object.sha1, "", &revs->pruning);
 
return retval >= 0 && (tree_difference == REV_TREE_SAME);
 }
-- 
1.9.rc1.181.g641f458

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

[PATCH 1/4] tree-diff: allow diff_tree_sha1 to accept NULL sha1

[Kirill Smelkov]

which would mean that corresponding tree - old or new - is empty.

As followup patches will show, that functionality was already needed in
several places of Git codebase, but there, we were preparing empty
tree_desc objects by hand, with some code duplication.

For handling sha1 = NULL case, let's reuse fill_tree_descriptor() which
returns just empty tree_desc in that case.

Signed-off-by: Kirill Smelkov 
---
 tree-diff.c | 12 
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index f7b3ade..f438478 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -287,14 +287,10 @@ int diff_tree_sha1(const unsigned char *old, const 
unsigned char *new, const cha
unsigned long size1, size2;
int retval;
 
-   tree1 = read_object_with_reference(old, tree_type, &size1, NULL);
-   if (!tree1)
-   die("unable to read source tree (%s)", sha1_to_hex(old));
-   tree2 = read_object_with_reference(new, tree_type, &size2, NULL);
-   if (!tree2)
-   die("unable to read destination tree (%s)", sha1_to_hex(new));
-   init_tree_desc(&t1, tree1, size1);
-   init_tree_desc(&t2, tree2, size2);
+   tree1 = fill_tree_descriptor(&t1, old);
+   tree2 = fill_tree_descriptor(&t2, new);
+   size1 = t1.size;
+   size2 = t2.size;
retval = diff_tree(&t1, &t2, base, opt);
if (!*base && DIFF_OPT_TST(opt, FOLLOW_RENAMES) && 
diff_might_be_rename()) {
init_tree_desc(&t1, tree1, size1);
-- 
1.9.rc1.181.g641f458

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

[PATCH 0/4] Teach diff_tree_sha1() to accept NULL sha1 for empty trees

[Kirill Smelkov]

Some preparatory patches for my reworked nparent tree-walker. Please apply.

Thanks beforehand,
Kirill

Kirill Smelkov (4):
  tree-diff: allow diff_tree_sha1 to accept NULL sha1
  tree-diff: convert diff_root_tree_sha1() to just call diff_tree_sha1
with old=NULL
  line-log: convert to using diff_tree_sha1()
  revision: convert to using diff_tree_sha1()

 line-log.c  | 26 ++
 revision.c  | 12 +---
 tree-diff.c | 27 +--
 3 files changed, 8 insertions(+), 57 deletions(-)

-- 
1.9.rc1.181.g641f458

--
To unsubscribe from this list: send the line "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 7/8] combine-diff: Fast changed-to-all-parents paths scanning

[Kirill Smelkov]

On Tue, Feb 04, 2014 at 10:37:24AM -0800, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> >> if we did not want to reinvent the whole tree walking thing, which
> >> would add risks for new bugs and burden to maintain this and the
> >> usual two-tree diff tree walking in sync.
> >
> > Junio, I understand it is not good to duplicate code and increase
> > maintenance risks
> > Instead I propose we switch to the new tree walker completely.
> 
> I am not fundamentally opposed to the idea. We'll see what happens.

Thanks. Locally, with draft patches with generalized tree-walker, for
`git log --raw --no-abbrev --no-renames` I was able to get to as fast as
with old two-tree walker for navy.git and 1% faster for linux.git and
all tests pass.

Only, before I clean things up, I'd like to ask - would the following
patch be accepted

 8< ---
diff --git a/tree-walk.c b/tree-walk.c
index 79dba1d..4dc86c7 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -37,7 +37,7 @@ static void decode_tree_entry(struct tree_desc *desc, const 
char *buf, unsigned
 
/* Initialize the descriptor entry */
desc->entry.path = path;
-   desc->entry.mode = mode;
+   desc->entry.mode = canon_mode(mode);
desc->entry.sha1 = (const unsigned char *)(path + len);
 }
 
diff --git a/tree-walk.h b/tree-walk.h
index ae04b64..ae7fb3a 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -16,7 +16,7 @@ struct tree_desc {
 static inline const unsigned char *tree_entry_extract(struct tree_desc *desc, 
const char **pathp, unsigned int *modep)
 {
*pathp = desc->entry.path;
-   *modep = canon_mode(desc->entry.mode);
+   *modep = desc->entry.mode;
return desc->entry.sha1;
 }
 8< ---
 
?

The reason I ask is because it is more handy to work with tree_desc
structures, compared to splitting it to sha1/path/mode/pathlen, and
if canon_mode() is done only once, clients don't have to pay the
performance price of canon_mode on each tree_desc "extract".

I understand there is fsck, which on invalid tree entry prints the mode,
but maybe somehow fsck should be special, and common interface be tuned
to usual clients?

Thanks,
Kirill
--
To unsubscribe from this list: send the line "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: bash completion patch

[Matthieu Moy]

乙酸鋰  writes:

> add --recurse-submodules

Thanks for the patch, but it cannot be included as-is.

Please, read Documentation/SubmittingPatches in Git's source tree. In
particular, the signed-off-by part. Also, don't use attachments to send
you patches (git send-email can help) and don't forget to Cc Junio if
you think your patch is ready for inclusion.

Thanks,

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

bash completion patch

[乙酸鋰]

add --recurse-submodules
From 0390a24e2653e0cdb6bfb9a569c28e4d58002038 Mon Sep 17 00:00:00 2001
From: Sup Yut Sum 
Date: Wed, 5 Feb 2014 23:09:46 +0800
Subject: [PATCH 1/1] bash completion: Add --recurse-submodules

---
 contrib/completion/git-completion.bash | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9525343..87de809 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1221,14 +1221,20 @@ _git_difftool ()
 	__git_complete_revlist_file
 }
 
+__git_fetch_recurse_submodules="yes on-demand no"
+
 __git_fetch_options="
 	--quiet --verbose --append --upload-pack --force --keep --depth=
-	--tags --no-tags --all --prune --dry-run
+	--tags --no-tags --all --prune --dry-run --recurse-submodules=
 "
 
 _git_fetch ()
 {
 	case "$cur" in
+	--recurse-submodules=*)
+		__gitcomp "$__git_fetch_recurse_submodules" "" "${cur##--recurse-submodules=}"
+		return
+		;;
 	--*)
 		__gitcomp "$__git_fetch_options"
 		return
@@ -1583,6 +1589,10 @@ _git_pull ()
 	__git_complete_strategy && return
 
 	case "$cur" in
+	--recurse-submodules=*)
+		__gitcomp "$__git_fetch_recurse_submodules" "" "${cur##--recurse-submodules=}"
+		return
+		;;
 	--*)
 		__gitcomp "
 			--rebase --no-rebase
@@ -1595,6 +1605,8 @@ _git_pull ()
 	__git_complete_remote_or_refspec
 }
 
+__git_push_recurse_submodules="check on-demand"
+
 _git_push ()
 {
 	case "$prev" in
@@ -1607,10 +1619,15 @@ _git_push ()
 		__gitcomp_nl "$(__git_remotes)" "" "${cur##--repo=}"
 		return
 		;;
+	--recurse-submodules=*)
+		__gitcomp "$__git_push_recurse_submodules" "" "${cur##--recurse-submodules=}"
+		return
+		;;
 	--*)
 		__gitcomp "
 			--all --mirror --tags --dry-run --force --verbose
 			--receive-pack= --repo= --set-upstream
+			--recurse-submodules=
 		"
 		return
 		;;
-- 
1.8.5.2

Re: [PATCH v2] userdiff: update Ada patterns

[Adrian Johnson]

On 04/02/14 06:30, Junio C Hamano wrote:
> Adrian Johnson  writes:
> 
>> - Allow extra space in "is new" and "is separate"
>> - Fix bug in word regex for numbers
>>
>> Signed-off-by: Adrian Johnson 
>> ---
>>  t/t4034/ada/expect | 2 +-
>>  userdiff.c | 4 ++--
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/t/t4034/ada/expect b/t/t4034/ada/expect
>> index be2376e..a682d28 100644
>> --- a/t/t4034/ada/expect
>> +++ b/t/t4034/ada/expect
>> @@ -4,7 +4,7 @@
>>  +++ b/post
>>  @@ -1,13 +1,13 @@
>>  Ada.Text_IO.Put_Line("Hello World!?");
>> -1 1e-10 16#FE12#E2 3.141_592 'xy'
>> +1 1e-101e10 16#FE12#E2 3.141_592 
>> 'xy'
>>  ax+b ay x-b
>>  ay
>>  x*b ay x/b
>> diff --git a/userdiff.c b/userdiff.c
>> index ea43a03..10b61ec 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -15,13 +15,13 @@ static int drivers_alloc;
>>word_regex "|[^[:space:]]|[\xc0-\xff][\x80-\xbf]+" }
>>  static struct userdiff_driver builtin_drivers[] = {
>>  IPATTERN("ada",
>> - "!^(.*[ \t])?(is new|renames|is separate)([ \t].*)?$\n"
>> + "!^(.*[ \t])?(is[ \t]+new|renames|is[ \t]+separate)([ \t].*)?$\n"
>>   "!^[ \t]*with[ \t].*$\n"
>>   "^[ \t]*((procedure|function)[ \t]+.*)$\n"
>>   "^[ \t]*((package|protected|task)[ \t]+.*)$",
>>   /* -- */
>>   "[a-zA-Z][a-zA-Z0-9_]*"
>> - "|[0-9][-+0-9#_.eE]"
>> + "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
> 
> This would match a lot wider than what I read you said you wanted to
> match in your previous message.  Does "-04##4_3_2Ee-9" count as a
> number, for example, or can we just ignore such syntactically
> incorrect sequence?

Maybe I am misunderstanding the purpose of the word diff regexes. I
thought the purpose of the word regex is to split lines into words, not
determine what is syntactically correct.

For example decimal number regex for pascal is: [-+0-9.e]+
and for cpp: [-+0-9.e]+[fFlL]?

These will obviously match stuff that is not a number.

> 
>>   "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
>>  IPATTERN("fortran",
>>   "!^([C*]|[ \t]*!)\n"
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH 2/8] tests: add checking that combine-diff emits only correct paths

[Kirill Smelkov]

On Mon, Feb 03, 2014 at 03:10:08PM -0800, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > where "correct paths" stands for paths that are different to all
> > parents.
> >
> > Up until now, we were testing combined diff only on one file, or on
> > several files which were all different (t4038-diff-combined.sh).
> >
> > As recent thinko in "simplify intersect_paths() further" showed, and
> > also, since we are going to rework code for finding paths different to
> > all parents, lets write at least basic tests.
> 
> Thanks.  Some nitpicks.
> 
> >
> > Signed-off-by: Kirill Smelkov 
> > ---
> >  t/t4057-diff-combined-paths.sh | 106 
> > +
> >  1 file changed, 106 insertions(+)
> >  create mode 100755 t/t4057-diff-combined-paths.sh
> >
> > diff --git a/t/t4057-diff-combined-paths.sh b/t/t4057-diff-combined-paths.sh
> > new file mode 100755
> > index 000..e6e457d
> > --- /dev/null
> > +++ b/t/t4057-diff-combined-paths.sh
> > @@ -0,0 +1,106 @@
> > +#!/bin/sh
> > +
> > +test_description='combined diff show only paths that are different to all 
> > parents'
> > +
> > +. ./test-lib.sh
> > +
> > +# verify that diffc.expect matches output of
> > +# `git diff -c --name-only HEAD HEAD^ HEAD^2`
> > +diffc_verify() {
> 
> Style: SP before (), i.e.
> 
>   diffc_verify () {
> 
> > +   git diff -c --name-only HEAD HEAD^ HEAD^2 >diffc.actual &&
> > +   test_cmp diffc.expect diffc.actual
> > +}
> > +
> > +test_expect_success 'trivial merge - combine-diff empty' '
> > +   for i in `test_seq 1 9`
> 
> Style: prefer $() over ``

Thanks, I've corrected the style.

> 
> > +   do
> > +   echo $i >$i.txt &&
> > +   git add $i.txt
> 
> Quiz.  What happens when this "git add" fails with an earlier value
> of $i?

We just continue and ignore the error = bad. On the other hand, there
are a lot of places like this one in git's testsuite, and almost almost
nobody cares. In a few places people add

|| exit

to last command, but I wonder, is there a way to omit the boilerplate,
and also || exit is not strictly correct, as sometime we would need to
analyse `for` exit code for good and also for bad, and exit just exits
from shell... What would be the most convenient thing to do here?

Please find patch with style issues corrected below

Thanks,
Kirill

 8< -
From: Kirill Smelkov 
Date: Mon, 3 Feb 2014 13:08:49 +0400
Subject: [PATCH] tests: add checking that combine-diff emits only correct paths

where "correct paths" stands for paths that are different to all
parents.

Up until now, we were testing combined diff only on one file, or on
several files which were all different (t4038-diff-combined.sh).

As recent thinko in "simplify intersect_paths() further" showed, and
also, since we are going to rework code for finding paths different to
all parents, lets write at least basic tests.

Signed-off-by: Kirill Smelkov 
---
 t/t4057-diff-combined-paths.sh | 106 +
 1 file changed, 106 insertions(+)
 create mode 100755 t/t4057-diff-combined-paths.sh

diff --git a/t/t4057-diff-combined-paths.sh b/t/t4057-diff-combined-paths.sh
new file mode 100755
index 000..097e632
--- /dev/null
+++ b/t/t4057-diff-combined-paths.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='combined diff show only paths that are different to all 
parents'
+
+. ./test-lib.sh
+
+# verify that diffc.expect matches output of
+# `git diff -c --name-only HEAD HEAD^ HEAD^2`
+diffc_verify () {
+   git diff -c --name-only HEAD HEAD^ HEAD^2 >diffc.actual &&
+   test_cmp diffc.expect diffc.actual
+}
+
+test_expect_success 'trivial merge - combine-diff empty' '
+   for i in $(test_seq 1 9)
+   do
+   echo $i >$i.txt &&
+   git add $i.txt
+   done &&
+   git commit -m "init" &&
+   git checkout -b side &&
+   for i in $(test_seq 2 9)
+   do
+   echo $i/2 >>$i.txt
+   done &&
+   git commit -a -m "side 2-9" &&
+   git checkout master &&
+   echo 1/2 >1.txt &&
+   git commit -a -m "master 1" &&
+   git merge side &&
+   >diffc.expect &&
+   diffc_verify
+'
+
+
+test_expect_success 'only one trully conflicting path' '
+   git checkout side &&
+   for i in $(test_seq 2 9)
+   do
+   echo $i/3 >>$i.txt
+   done &&
+   echo "4side" >>4.txt &&
+   git commit -a -m "side 2-9 +4" &&
+   git checkout master &&
+   for i in $(test_seq 1 9)
+   do
+   echo $i/3 >>$i.txt
+   done &&
+   echo "4master" >>4.txt &&
+   git commit -a -m "master 1-9 +4" &&
+   test_must_fail git merge side &&
+   cat <<-\EOF >4.txt &&
+   4
+   4/2
+   4/3
+   4master
+   4side
+   EOF
+   git add 4.txt &&
+   git commit -m "merge side (2)" &&
+   echo 4.txt >diffc.expect &&
+   diffc_verify
+'
+
+test_expect_success 'merge introduces new file' '
+   git checkout side &&
+

Re: [PATCH] blame.c: prepare_lines should not call xrealloc for every line

[David Kastrup]

Junio C Hamano  writes:

> which I think is the prevalent style in our codebase.  The same for
> the other loop we see in the new code below.
>
>  - avoid assignments in conditionals when you do not have to.

commit a77a48c259d9adbe7779ca69a3432e493116b3fd
Author: Junio C Hamano 
Date:   Tue Jan 28 13:55:59 2014 -0800

combine-diff: simplify intersect_paths() further
[...]

+   while ((p = *tail) != NULL) {

Because we can.

At any rate, I am not going to put any more work into this patch as it
is decidedly not worth the bad taste this discussion leaves in my mouth.
And I don't want any code marked as written by me that does not
correspond to what I'd be willing to write.  So please make sure to put
any rewrites in a separate commit with different authorship.

Thanks

-- 
David Kastrup

--
To unsubscribe from this list: send the line "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] blame.c: prepare_lines should not call xrealloc for every line

[David Kastrup]

Junio C Hamano  writes:

> David Kastrup  writes:
>
>>> so something like
>>>
>>> for (p = buf; p < end; p++) {
>>> p = find the end of this line
>>> if (!p)
>>> break;
>>> num++;
>>> }
>>>
>>> perhaps?
>>
>> Would crash on incomplete last line.
>
> Hmph, even with "if !p"?

Admitted.  So we have one _proper_ loop termination condition in the
middle of the loop, and we have a snake oil condition at the start of
the loop that can, according to the standard, _only_ yield true reliably
when p == end (any end > p can only result from undefined behavior when
p points to an object of size end - p).

The effect of this condition basically is to state "we don't trust
memchr to do the right thing when called with 0 as its last argument
which can happen at most once".  This condition will come about by the
_combined_ effect of p = ... _in_ the loop (which is the real iteration
advancing p) and p++ hidden in the for statement (which never makes any
sense separated from the p = ...).

It turns out that
a) memchr is provided by a compatibility layer in case it is missing or
   defective
b) other code in Git demands that memchr works correctly with a zero
   last argument.  See, for example, attr.c:
if (dirlen <= len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);

This clearly needs to be able to work with dirlen == len + 1.

So the loop gets rewritten in a manner where the for statement
_pretends_ to loop linearly through buf, but where the loop _body_ has
its own _regular_ termination condition it shields from the original
for, and p is advanced _independently_.  The advancement of p to beyond
the next '\n' is distributed to two different places in the loop, and
one place is made to look as if it does something else.

> But that last round of the loop will be no-op, so "p < end" vs "p <=
> end" does not make any difference.  It is not even strictly
> necessary because memchr() limits the scan to end, but it would
> still be a good belt-and-suspenders defensive coding practice, I
> would think.

It's snake oil making debugging harder.  It masks the real action, and
it will route around suspected faulty behavior that is wrong according
to the standards and not permitted elsewhere in the codebase.  Other
than that, it just takes additional performance from every iteration.

Putting belts and suspenders on a bike looks comforting until the
suspenders get caught in the wheelspokes.

> which is the same as
>
>   for (p = buf; ; p++) {
>   *lineno++ = p - buf;
> p = memchr...
> if (!p)
>   break;
>   }
>
> and the latter has the loop control p++ at where it belongs to.

But it's only half the control.

As it is clear that we won't get to a state where I'd be willing to have
"git blame" pointing to me as the author, I suggest that you either put
your belts and suspenders on with a separate commit under your own
authorship, or that we drop this altogether.

As I stated already: this patch was just provided because the original
code offends my sense of aesthetics, so there is no point to it if it
offends yours.

I'd still recommend fixing the sizeof(int *) with sizeof(int) confusion.

> If we wanted to have a belt-and-suspenders safety, we need "p <=
> end" here, not "p < end",

That would be totally ridiculous since end > p cannot ever happen except
by undefined behavior.  Pointer inequalities require both pointers to be
associated with the same object.

> This was fun ;-)

At the expense of seriously impacting my motivation to do any further
code cleanup on Git.

Which is a reasonable tradeoff since your fun is more important to Git's
well-being than my one.

-- 
David Kastrup

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