Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Junio C Hamano]

Duy Nguyen  writes:

> I think this applies to general case as well, not just shallow.
> Imagine I have a disconnected commit that points to the latest tree
> (i.e. it contains most of latest changes). Because it's disconnected,
> it'll be ignored by the server side. But if the servide side does
> mark_tree_interesting on this commit, a bunch of blobs might be
> excluded from sending.

I think you meant mark_tree_UNinteresting.

> ... So perhaps we could go over have_obj list
> again, if it's not processed and is
>
>  - a tree-ish, mark_tree_uninteresting
>  - a blob, just mark unintesting
>
> and this does regardless of shallow state or edges.

As a general idea, I agree it may be worth trying out to see if your
concern that the "have" list may be so big that this approach may be
more costly than it is worth.

If the recipient is known to have something, we do not have to send
it.

The things that we decide not to send are not necessarily what the
recipient has, which introduces a twist you need to watch out for if
we want to go that route.

If the recipient is known to have something, a thin transfer can
send a delta against it.  You do not want to send the commits before
the shallow boundary (i.e. the parents of the commits listed in
.git/shallow) because the recipient does not want them, and that
means you may have to use a different mark to record that fact.  The
recipient does not have them, we do not want to send them, and they
cannot be used as a delta base for what we do send.  Which is quite
different from the ordinary "uninteresting" objects, those we decide
not to send because the recipient has them.

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

Re: [PATCH 3/4] pack-objects: do not print usage when repacking

[Antoine Pelisse]

On Wed, Aug 7, 2013 at 4:00 PM, Stefan Beller
 wrote:
> ---
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 1742ea1..0bd8f3b 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -2585,7 +2585,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
> char *prefix)
> base_name = argv[0];
> argc--;
> }
> -   if (pack_to_stdout != !base_name || argc)
> +   if (!(repack_flags & REPACK_IN_PROGRESS) && (pack_to_stdout != 
> !base_name || argc))
> usage_with_options(pack_usage, pack_objects_options);
>
> rp_av[rp_ac++] = "pack-objects";

Hello Stefan,
I'm not sure I understand that and why it's needed, would you mind
explain it to me ? (and/or maybe add it to the commit message)

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: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Junio C Hamano]

Duy Nguyen  writes:

> Haven't found time to read the rest yet, but this I can answer.
> .git/shallow records graft points. If a commit is in .git/shallow and
> it exists in the repository, the commit is considered to have no
> parents regardless of what's recorded in repository. So .git/shallow
> refers to the new roots, not the missing bits.

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: Remove old forgotten command: whatchanged

[Junio C Hamano]

Stefan Beller  writes:

> Well if we make sure the whatchanged command can easily be reproduced
> with the log command, we could add the missing parameters to it, hence
> no change for the user. (git whatchanged == git log --raw --no-merges or
> git log --wc [to be done yet]).
>
> So I did not mean to introduce a change for users!

I certainly did *not* read that from between the lines of what you
wrote:

+ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
+ {
+   return cmd_log(argc, argv, prefix)
+ }

In principle, I agree that it is a good idea to try to share enough
code, while keeping the flexiblity and clarity of the code for
maintainability.

In the extreme, you could rewrite these two functions like so:

static int cmd_lw_helper(
int argc, const char **argv,
const char *prefix,
int whoami) 
{
struct rev_info rev;
struct setup_revision_opt opt;

init_grep_defaults();
git_config(git_log_config, NULL);
init_revisions(&rev, prefix);
if (whoami == 'l') { /* log */
rev.always_show_header = always_show_header;
} else { /* whatchanged */
rev.diff = diff;
rev.simplify_history = simplify_history;
}
memset(opt, 0, sizeof(opt));
opt.def = "HEAD";
opt.revarg_opt = REVARG_COMMITTISH;
cmd_log_init(argc, argv, prefix, &rev, &opt);
if (whoami == 'w') {
if (!rev.diffopt.output_format)
rev.diffopt.output_format = DIFF_FORMAT_RAW;
}
return cmd_log_walk(&rev);
}

int cmd_log(int argc, const char **argv, const char *prefix)
{
return cmd_lw_helper(argc, argv, prefix, 'l');
}

int cmd_whatchanged(int argc, const char **argv, const char *prefix)
{
return cmd_lw_helper(argc, argv, prefix, 'w');
}

but at that point, the cost you have to pay when you need to update
one of them but not the other becomes higher.

As whatchanged is kept primarily for people who learned Git by word
of mouth reading the kernel mailing list and are used to that
command.  Its external interface and what it does is not likely to
drastically change.  On the other hand, "log" is a primary Porcelain
and we would expect constant improvements.

Between the "share more code for reuse" and "keep the flexibility
and clarity for maintainability", it is a subtle balancing act.
Personally I think the current code strikes a good balance by not
going to the extreme, given that "change one (i.e. log) but not the
other" is a very likely pattern for the evolution of these two
commands.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] status: always show tracking branch even no change

[Jiang Xin]

If the current branch has an upstream branch, and there are changes
between the current branch and its upstream, some commands (such as
"git status", "git status -bs", and "git checkout") will report their
relationship. E.g.

$ git status
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#   (use "git push" to publish your local commits)
#
...

$ git status -bs
## master...origin/master [ahead 1]
...

$ git checkout master
Already on 'master'
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

But if there is no difference between the current branch and its
upstream, the relationship will not be reported, and it's hard to
tell whether the current branch has a tracking branch or not. And
what's worse, when the 'push.default' config variable is set to
`matching`, it's hard to tell whether the current branch has already
been pushed out or not at all [1].

With this patch, "git status" will report relationship between the
current branch and its upstream counterpart even if there is no
difference.

$ git status
# On branch master
# Your branch is identical to 'origin/master'.
#
...

$ git status -bs
## master...origin/master
...

$ git checkout master
Already on 'master'
Your branch is identical to 'origin/master'.

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

Signed-off-by: Jiang Xin 
---
 remote.c | 22 ++--
 t/t6040-tracking-info.sh | 54 
 wt-status.c  | 13 +---
 3 files changed, 76 insertions(+), 13 deletions(-)

diff --git a/remote.c b/remote.c
index 2433467..825f278 100644
--- a/remote.c
+++ b/remote.c
@@ -1740,6 +1740,10 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
const char *rev_argv[10], *base;
int rev_argc;
 
+   /* Set both num_theirs and num_ours as undetermined. */
+   *num_theirs = -1;
+   *num_ours = -1;
+
/*
 * Nothing to report unless we are marked to build on top of
 * somebody else.
@@ -1758,14 +1762,16 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
theirs = lookup_commit_reference(sha1);
if (!theirs)
return 0;
+   *num_theirs = 0;
 
if (read_ref(branch->refname, sha1))
return 0;
ours = lookup_commit_reference(sha1);
if (!ours)
return 0;
+   *num_ours = 0;
 
-   /* are we the same? */
+   /* are we the same? both num_theirs and num_ours have been set to 0. */
if (theirs == ours)
return 0;
 
@@ -1786,8 +1792,6 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
prepare_revision_walk(&revs);
 
/* ... and count the commits on each side. */
-   *num_ours = 0;
-   *num_theirs = 0;
while (1) {
struct commit *c = get_revision(&revs);
if (!c)
@@ -1812,12 +1816,18 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
int num_ours, num_theirs;
const char *base;
 
-   if (!stat_tracking_info(branch, &num_ours, &num_theirs))
-   return 0;
+   if (!stat_tracking_info(branch, &num_ours, &num_theirs)) {
+   if (num_ours || num_theirs)
+   return 0;
+   }
 
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
-   if (!num_theirs) {
+   if (!num_ours && !num_theirs) {
+   strbuf_addf(sb,
+   _("Your branch is identical to '%s'.\n"),
+   base);
+   } else if (!num_theirs) {
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
   "Your branch is ahead of '%s' by %d commits.\n",
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index ec2b516..eafce7d 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -28,18 +28,20 @@ test_expect_success setup '
git reset --hard HEAD^ &&
git checkout -b b4 origin &&
advance e &&
-   advance f
+   advance f &&
+   git checkout -b b5 origin
) &&
git checkout -b follower --track master &&
advance g
 '
 
-script='s/^..\(b.\)[0-9a-f]*\[\([^]]*\)\].*/\1 \2/p'
+script='s/^..\(b.\)[0-9a-f]*\(\[\([^]]*\)\]\)\{0,1\}.*/\1 \3/p'
 cat >expect <<\EOF
 b1 ahead 1, behind 1
 b2 ahead 1, behind 1
 b3 behind 1
 b4 ahead 2
+b5 
 EOF
 
 test_expect_success 'branch -v' '
@@ -56,6 +58,7 @@ b1 origin/master: ahead 1, behind 1
 b2 origin/master: ahead 1, behind 1
 b3 origin/master: behind 1
 b4 origin/master: ahead 2
+b5 origin/master
 EOF
 
 test_expect_success 'bra

Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Duy Nguyen]

On Fri, Jul 12, 2013 at 5:01 AM, Matthijs Kooijman  wrote:
> Hi folks,
>
> while playing with shallow fetches, I've found that in some
> circumstances running git fetch with --depth can return too many objects
> (in particular, _all_ the objects for the requested revisions are
> returned, even when some of those objects are already known to the
> client).
>
> This happens when a client issues a fetch with a depth bigger or equal
> to the number of commits the server is ahead of the client. In this
> case, the revisions to be sent over will be completely detached from any
> revisions the client already has (history-wise), causing the server to
> effectively ignore all objects the client has (as advertised using its
> have lines) and just send over _all_ objects (needed for the revisions
> it is sending over).
>
> I've traced this down to the way do_rev_list in upload-pack.c works. If
> I've poured over the code enough to understand it, this is what happens:
>  - The new shallow roots are made into graft points without parents.
>  - The "want" commits are added to the pending list (revs->pending)
>  - The "have" commits are marked uninteresting and added to the pending list
>  - prepare_revision_walk is called, which adds everything from the
>pending list into the commmit list (revs->commits)
>  - limit_list is called, which traverses the history of each interesting
>commit in the commit list (i.e., all want revisions), up to excluding
>the first uninteresting commit (i.e. a have revision). The result of
>this is the new commit list.
>
>This means the commit list now contains all commits that the client
>wants, up to (excluding) any commits he already has or up to
>(including) any (new) shallow roots.
>  - mark_edges_uninteresting is called, which marks the tree of every
>parent of each edge in the commit list as uninteresting (in practice,
>this marks the tree of each uninteresting parent, since those are by
>definition the only kinds of revisions that can be beyond the edge).
>  - All trees and blobs that are referenced by trees in the commit list
>but are not marked as uninteresting, are passed to git-pack-objects
>to put into the pack.
>
> Normally, the list of commits to send over is connected to the
> client's existing commits (which are marked as uninteresting). This
> means that only the trees of those uninteresting ("have") commits that
> are actually (direct) predecessors of the commits to send over are
> marked as uninteresting. This is probably useful, since it prevents
> having to go over all trees the client has (for other branches, for
> example) and instead limits to the trees that are the most likely to
> contain duplicate (or similar, for delta-ing) objects.
>
> However, in the "detached shallow fetch" case, this assumption is no
> longer valid. There will be no uninteresting commits as parents for
> the commit list, since all edge commits will be shallow roots (hence
> have no parents).  Ideally, one would find out which of the "detached"
> "have" revisions are the closest to the new shallow roots, but with the
> current code these shallow roots have their parents cut off long before
> this code even runs, so this is probably not feasible.

I think this applies to general case as well, not just shallow.
Imagine I have a disconnected commit that points to the latest tree
(i.e. it contains most of latest changes). Because it's disconnected,
it'll be ignored by the server side. But if the servide side does
mark_tree_interesting on this commit, a bunch of blobs might be
excluded from sending. I used to (ab)use git and store a bunch of tags
point to trees. These trees share a lot. Still, fetching a new tag
means pulling all objects of the new tree even though it only needs a
few new blobs and trees. So perhaps we could go over have_obj list
again, if it's not processed and is

 - a tree-ish, mark_tree_uninteresting
 - a blob, just mark unintesting

and this does regardless of shallow state or edges. The only downside
is mark_tree_uninteresting is recursive so in unpacks lots of trees if
have_obj is long, or the worktree is really big. Commit bitmap should
help reduce the cost if have_obj is a committish, at least.
-- 
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: Remove old forgotten command: whatchanged

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The only thing it does is to scratch an irrelevant itch by people
> who peek the codebase and find an old command whose existence does
> not even hurt them.  They may have too much time on their hand, but
> that is not an excuse to waste other people's time by adding extra
> makework on our plate, or changing the behaviour for people who use
> the command without explanation.

It's a maintenance burden that confuses users. I'd encourage you to
read git-whatchanged(1), and tell me why nobody has updated it.

We're not maintaining a museum.
--
To unsubscribe from this list: send the line "unsubscribe 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/4] backup_file dummy function

[Duy Nguyen]

On Wed, Aug 7, 2013 at 9:00 PM, Stefan Beller
 wrote:
> ---
>  pack-write.c | 5 +
>  1 file changed, 5 insertions(+)
>
> diff --git a/pack-write.c b/pack-write.c
> index e6aa7e3..b728ea2 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -344,6 +344,11 @@ struct sha1file *create_tmp_packfile(char 
> **pack_tmp_name)
> return sha1fd(fd, *pack_tmp_name);
>  }
>
> +void backup_file(char *filename)
> +{
> +
> +}
> +

I think the function looks like this (before it got lost after rebase)

+static void backup_file(const char *path)
+{
+   struct stat st;
+   if (repack_flags & REPACK_IN_PROGRESS &&
+   !stat(path, &st)) {
+   struct strbuf old = STRBUF_INIT;
+   strbuf_addf(&old, "%s.old", path);
+   if (rename(path, old.buf))
+   die_errno("unable to rename pack %s", path);
+   string_list_append(&backup_files, strbuf_detach(&old, NULL));
+   }
+}
+
-- 
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 1/4] Build in git-repack

[Duy Nguyen]

On Wed, Aug 7, 2013 at 10:48 PM, Junio C Hamano  wrote:
> Matthieu Moy  writes:
>
>> [ It's cool you're working on this, I'd really like a git-repack in C.
>>   That would fix this
>>   http://thread.gmane.org/gmane.comp.version-control.git/226458 ]
>>
>> Stefan Beller  writes:
>>
>>> From: Nguyễn Thái Ngọc Duy 
>>>
>>> pack-objects learns a few more options to take over what's been done
>>> by git-repack.sh. cmd_repack() becomes a wrapper around
>>> cmd_pack_objects().
>>
>> I think the patch would read easier if these were split into two
>> patches: one doing the real stuff in pack-objects, and then getting rid
>> of git-repack.sh to replace it with a trivial built-in.
>>
>> Actually, I'm wondering why pack-objects requires so much changes.
>> git-repack.sh was already a relatively small wrapper around
>> pack-objects, and did not need the new options you add, so why are they
>> needed? In particular adding the new --update-info option that just does
>>
>>> +if (repack_flags & REPACK_UPDATE_INFO)
>>> +update_server_info(0);
>>
>> seems overkill to me: why don't you just let cmd_repack call
>> update_server_info(0)?
>
> My feeling exactly.  I would rather see a patch that does not touch
> pack-objects at all, and use run_command() interface to spawn it.
> Once we do have to pack, the necessary processing cycle will dwarf
> the fork/exec latency anyway, no?

I'm not opposed to run_command(). I think the reason I wanted to move
repack functionality to pack-objects is to avoid reading sha-1 from
pack-objects and reconstruct the paths again from the sha-1. But for
simplicity, perhaps we should not touch pack-objects at all. Then we
could have builtin/repack.c instead of stuffing cmd_repack in
builtin/pack-objects.c

@Stefan, if you want to push this work, feel free to take it as _your_
patch, rewrite as will. You don't need to retain my name.
-- 
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] git exproll: steps to tackle gc aggression

[Duy Nguyen]

On Wed, Aug 7, 2013 at 7:10 AM, Martin Fick  wrote:
>> I wonder if a simpler approach may be nearly efficient as
>> this one: keep the largest pack out, repack the rest at
>> fetch/push time so there are at most 2 packs at a time.
>> Or we we could do the repack at 'gc --auto' time, but
>> with lower pack threshold (about 10 or so). When the
>> second pack is as big as, say half the size of the
>> first, merge them into one at "gc --auto" time. This can
>> be easily implemented in git-repack.sh.
>
> It would definitely be better than the current gc approach.
>
> However, I suspect it is still at least one to two orders of
> magnitude off from where it should be.  To give you a real
> world example, on our server today when gitexproll ran on
> our kernel/msm repo, it consolidated 317 pack files into one
> almost 8M packfile (it compresses/dedupes shockingly well,
> one of those new packs was 33M).  Our largest packfile in
> that repo is 1.5G!
>
> So let's now imagine that the second closest packfile is
> only 100M, it would keep getting consolidated with 8M worth
> of data every day (assuming the same conditions and no extra
> compression).  That would take (750M-100M)/8M ~ 81 days to
> finally build up large enough to no longer consolidate the
> new packs with the second largest pack file daily.  During
> those 80+ days, it will be on average writing 325M too much
> per day (when it should be writing just 8M).
>
> So I can see the appeal of a simple solution, unfortunately
> I think one layer would still "suck" though.  And if you are
> going to add even just one extra layer, I suspect that you
> might as well go the full distance since you probably
> already need to implement the logic to do so?

I see. It looks like your way is the best way to go.
-- 
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 v2 12/19] read-cache: read index-v5

[Duy Nguyen]

On Wed, Aug 7, 2013 at 3:23 PM, Thomas Gummerer  wrote:
>> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer  
>> wrote:
>>> +static void ce_queue_push(struct cache_entry **head,
>>> +struct cache_entry **tail,
>>> +struct cache_entry *ce)
>>> +{
>>> +   if (!*head) {
>>> +   *head = *tail = ce;
>>> +   (*tail)->next = NULL;
>>> +   return;
>>> +   }
>>> +
>>> +   (*tail)->next = ce;
>>> +   ce->next = NULL;
>>> +   *tail = (*tail)->next;
>>
>> No no no. "next" is for name-hash.c don't "reuse" it here. And I don't
>> think you really need to maintain a linked list of cache_entries by
>> the way. More on read_entries comments below..
>
> You're right, I don't need it for the reading code.  I do need to keep a
> list of cache-entries for writing though, where a linked list is best
> suited for the task.  I'll use a next_ce pointer for that.

Hmm.. yeah you need to maintain temporary structures before writing
out direntries, then fileentries table. We can't just write as we
traverse through the cache in one go. Maybe a new field in cache_entry
for the linked list, or maintain the linked list off cache_entry..
whichever is easier for you.

> I've tried implementing both versions with the internal tree structure,
> see below for timings.
>
> simpleapi is the code that was posted to the list, HEAD uses a
> tree-structure for the directories internally, and replaces strcmp with
> cache_name_compare and leaves out the prefix.  This tree uses dummy
> entries for the sub-directories.
>
> Dummy entries are single NUL-bytes mixed with the cache-entries, which
> should give optimal performance.  I haven't thought much about
> corruption of the index, but I don't think we would need any additional
> crc checksums or similar.
>
> The performance advantage seems very slim to none when using the dummy
> entries to avoid the comparison though, so I don't think it makes sense
> to make the index more complicated for a very small speed-up.

Agreed.

> Testsimpleapi HEAD
>  this tree
> -
> 0003.2: v[23]: update-index 3.22(2.61+0.58)   3.20(2.56+0.61) 
> -0.6%3.22(2.67+0.53) +0.0%
> 0003.3: v[23]: grep nonexistent -- subdir   1.65(1.36+0.27)   1.65(1.34+0.30) 
> +0.0%1.67(1.34+0.31) +1.2%
> 0003.4: v[23]: ls-files -- subdir   1.50(1.20+0.29)   1.54(1.18+0.34) 
> +2.7%1.53(1.22+0.30) +2.0%
> 0003.6: v4: update-index2.69(2.28+0.39)   2.70(2.21+0.47) 
> +0.4%2.70(2.27+0.41) +0.4%
> 0003.7: v4: grep nonexistent -- subdir  1.33(0.98+0.34)   1.36(1.01+0.33) 
> +2.3%1.34(1.03+0.30) +0.8%
> 0003.8: v4: ls-files -- subdir  1.21(0.86+0.33)   1.23(0.91+0.30) 
> +1.7%1.22(0.90+0.30) +0.8%
> 0003.10: v5: update-index   2.12(1.58+0.52)   2.20(1.67+0.52) 
> +3.8%2.19(1.66+0.51) +3.3%
> 0003.11: v5: ls-files   1.57(1.21+0.34)   1.55(1.20+0.33) 
> -1.3%1.52(1.21+0.29) -3.2%
> 0003.12: v5: grep nonexistent -- subdir 0.08(0.06+0.01)   0.07(0.04+0.02) 
> -12.5%   0.07(0.04+0.02) -12.5%
> 0003.13: v5: ls-files -- subdir 0.07(0.04+0.01)   0.06(0.05+0.00) 
> -14.3%   0.07(0.05+0.01) +0.0%
-- 
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 v2 12/19] read-cache: read index-v5

[Duy Nguyen]

On Wed, Jul 17, 2013 at 3:11 PM, Thomas Gummerer  wrote:
> Duy Nguyen  writes:
>
> [..]
>
>>> +static int read_entries(struct index_state *istate, struct directory_entry 
>>> **de,
>>> +   unsigned int *entry_offset, void **mmap,
>>> +   unsigned long mmap_size, unsigned int *nr,
>>> +   unsigned int *foffsetblock)
>>> +{
>>> +   struct cache_entry *head = NULL, *tail = NULL;
>>> +   struct conflict_entry *conflict_queue;
>>> +   struct cache_entry *ce;
>>> +   int i;
>>> +
>>> +   conflict_queue = NULL;
>>> +   if (read_conflicts(&conflict_queue, *de, mmap, mmap_size) < 0)
>>> +   return -1;
>>> +   for (i = 0; i < (*de)->de_nfiles; i++) {
>>> +   if (read_entry(&ce,
>>> +  *de,
>>> +  entry_offset,
>>> +  mmap,
>>> +  mmap_size,
>>> +  foffsetblock) < 0)
>>> +   return -1;
>>> +   ce_queue_push(&head, &tail, ce);
>>> +   *foffsetblock += 4;
>>> +
>>> +   /*
>>> +* Add the conflicted entries at the end of the index file
>>> +* to the in memory format
>>> +*/
>>> +   if (conflict_queue &&
>>> +   (conflict_queue->entries->flags & CONFLICT_CONFLICTED) 
>>> != 0 &&
>>> +   !cache_name_compare(conflict_queue->name, 
>>> conflict_queue->namelen,
>>> +   ce->name, ce_namelen(ce))) {
>>> +   struct conflict_part *cp;
>>> +   cp = conflict_queue->entries;
>>> +   cp = cp->next;
>>> +   while (cp) {
>>> +   ce = convert_conflict_part(cp,
>>> +   conflict_queue->name,
>>> +   conflict_queue->namelen);
>>> +   ce_queue_push(&head, &tail, ce);
>>> +   conflict_part_head_remove(&cp);
>>> +   }
>>> +   conflict_entry_head_remove(&conflict_queue);
>>> +   }
>>
>> I start to wonder if separating staged entries is a good idea. It
>> seems to make the code more complicated. The good point about conflict
>> section at the end of the file is you can just truncate() it out.
>> Another way is putting staged entries in fileentries, sorted
>> alphabetically then by stage number, and a flag indicating if the
>> entry is valid. When you remove resolve an entry, just set the flag to
>> invalid (partial write), so that read code will skip it.
>>
>> I think this approach is reasonably cheap (unless there are a lot of
>> conflicts) and it simplifies this piece of code. truncate() may be
>> overrated anyway. In my experience, I "git add " as soon as I
>> resolve  (so that "git diff" shrinks). One entry at a time, one
>> index write at a time. I don't think I ever resolve everything then
>> "git add -u .", which is where truncate() shines because staged
>> entries are removed all at once. We should optimize for one file
>> resolution at a time, imo.
>
> Thanks for your comments.  I'll address the other ones once we decided
> to do with the conflicts.
>
> It does make the code quite a bit more complicated, but also has one
> advantage that you overlooked.

I did overlook, although my goal is to keep the code simpler, not more
comlicated. The thinking is if we can find everything in fileentries
table, the code here is simplified, so..

> We wouldn't truncate() when resolving
> the conflicts.  The resolve undo data is stored with the conflicts and
> therefore we could just flip a bit and set the stage of the cache-entry
> in the main index to 0 (always once we got partial writing).  This can
> be fast both in case we resolve one entry at a time and when we resolve
> a lot of entries.  The advantage is even bigger when we resolve one
> entry at a time, when we otherwise would have to re-write the index for
> each conflict resolution.

If I understand it correctly, filentries can only contain stage-0 or
stage-1 entries, "stage > 0" entries are stored in conflict data. Once
a conflict is solved, you update the stage-1 entry in fileentries,
turning it to stage-0 and recalculate the entry checksum. Conflict
data remains there to function as the old REUC extension. Correct?

First of all, if that's true, we only need 1 bit for stage in fileentries table.

Secondly, you may get away with looking up to conflict data in this
function by storing all stages in fileentries (now we need 2-bit
stage), replicated in conflict data for reuc function. When you
resolve conflict, you flip stage-1 to stage-0, and flip (a new bit) to
mark stage-2 entry invalid so the code knows to skip it. Next time the
index is rewritten, invalid entr

Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Duy Nguyen]

On Thu, Aug 8, 2013 at 8:01 AM, Junio C Hamano  wrote:
> Matthijs Kooijman  writes:
>
>>> > In your discussion (including the comment), you talk about "shallow
>>> > root" (I think that is the same as what we call "shallow boundary"),
>>> I think so, yes. I mean to refer to the commits referenced in
>>> .git/shallow, that have their parents "hidden".
>> Could you confirm that I got the terms right here (or is the shallow
>> boundary the first hidden commit?)
>
> As long as you are consistent it is fine. I _think_ boundary refers
> to what is recorded in the .git/shallow file, so they are commits
> that are missing from our repository, and their immediate children
> are available.

Haven't found time to read the rest yet, but this I can answer.
.git/shallow records graft points. If a commit is in .git/shallow and
it exists in the repository, the commit is considered to have no
parents regardless of what's recorded in repository. So .git/shallow
refers to the new roots, not the missing bits.
-- 
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: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Junio C Hamano]

Matthijs Kooijman  writes:

>> > In your discussion (including the comment), you talk about "shallow
>> > root" (I think that is the same as what we call "shallow boundary"),
>> I think so, yes. I mean to refer to the commits referenced in
>> .git/shallow, that have their parents "hidden".
> Could you confirm that I got the terms right here (or is the shallow
> boundary the first hidden commit?)

As long as you are consistent it is fine. I _think_ boundary refers
to what is recorded in the .git/shallow file, so they are commits
that are missing from our repository, and their immediate children
are available.

> My proposal was to only apply the fix for all have revisions when the
> previous history traversal came across some shallow boundary commits. If
> this happens, then that shallow boundary commit will be a "new" one and
> it will have prevented the history traversal from finding the full list
> of relevant "have" commits. In this case, we should just use all "have"
> commits instead.
>
> Now, looking at the code, I see a few options for detecting this case:
>
>  1 Modify mark_edges_uninteresting to return a boolean (or have an
>output argument) if any of the commits in the list of commits to find
>(not the edges) is a shallow boundary.
>  2 Modify mark_edges_uninteresting to have a "show_shallow" argument
>that gets called for every shallow boundary. The show_shallow
>function passed would then simply keep a boolean if it is passed at
>least once.
>  3 Add another loop over the commits _after_ the call to
>mark_edges_uninteresting, that simply looks for any shallow boundary
>commit.
>
> The last option seems sensible to me, since it prevents modifying the
> somewhat generic mark_edges_uninteresting function for this specific
> usecase. On the other hand, it does mean that the list of commits is
> looped twice, not sure what that means for performance.
>
> Before I go and implement one of these, which option seems best to you?

My gut feeling without looking at any patch is that the simplest
(i.e. 3.) would be the best among these three.

But I suspect, with any of these approaches, you would need to be
very careful futzing with the edge ones.  It may have an interesting
interactions with --thin transfer.


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

[PATCH 2/5] fetch: rename file-scope global "transport" to "gtransport"

[Junio C Hamano]

Although many functions in this file take a "struct transport" as a
parameter, "fetch_one()" assigns to the global singleton instance
which is a file-scope static, in order to allow a parameterless
signal handler unlock_pack() to access it.

Rename the variable to gtransport to make sure these uses stand out.

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

diff --git a/builtin/fetch.c b/builtin/fetch.c
index d784b2e..636b47f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -36,7 +36,7 @@ static int tags = TAGS_DEFAULT, unshallow;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
-static struct transport *transport;
+static struct transport *gtransport;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 
@@ -95,8 +95,8 @@ static struct option builtin_fetch_options[] = {
 
 static void unlock_pack(void)
 {
-   if (transport)
-   transport_unlock_pack(transport);
+   if (gtransport)
+   transport_unlock_pack(gtransport);
 }
 
 static void unlock_pack_on_signal(int signo)
@@ -818,13 +818,13 @@ static int do_fetch(struct transport *transport,
 
 static void set_option(const char *name, const char *value)
 {
-   int r = transport_set_option(transport, name, value);
+   int r = transport_set_option(gtransport, name, value);
if (r < 0)
die(_("Option \"%s\" value \"%s\" is not valid for %s"),
-   name, value, transport->url);
+   name, value, gtransport->url);
if (r > 0)
warning(_("Option \"%s\" is ignored for %s\n"),
-   name, transport->url);
+   name, gtransport->url);
 }
 
 static int get_one_remote_for_fetch(struct remote *remote, void *priv)
@@ -949,8 +949,8 @@ static int fetch_one(struct remote *remote, int argc, const 
char **argv)
die(_("No remote repository specified.  Please, specify either 
a URL or a\n"
"remote name from which new revisions should be fetched."));
 
-   transport = transport_get(remote, NULL);
-   transport_set_verbosity(transport, verbosity, progress);
+   gtransport = transport_get(remote, NULL);
+   transport_set_verbosity(gtransport, verbosity, progress);
if (upload_pack)
set_option(TRANS_OPT_UPLOADPACK, upload_pack);
if (keep)
@@ -983,10 +983,10 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
sigchain_push_common(unlock_pack_on_signal);
atexit(unlock_pack);
refspec = parse_fetch_refspec(ref_nr, refs);
-   exit_code = do_fetch(transport, refspec, ref_nr);
+   exit_code = do_fetch(gtransport, refspec, ref_nr);
free_refspec(ref_nr, refspec);
-   transport_disconnect(transport);
-   transport = NULL;
+   transport_disconnect(gtransport);
+   gtransport = NULL;
return exit_code;
 }
 
-- 
1.8.4-rc1-210-gf6d87e2

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

[PATCH 4/5] fetch: refactor code that fetches leftover tags

[Junio C Hamano]

Usually the upload-pack process running on the other side will give
us all the reachable tags we need during the primary object transfer
in do_fetch().  If that does not happen (e.g. the other side may be
running a third-party implementation of upload-pack), we will run
another fetch to pick up leftover tags that we know point at the
commits reachable from our updated tips.

Separate out the code to run this second fetch into a helper
function.

Signed-off-by: Junio C Hamano 
---
 builtin/fetch.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 39a3fc8..0b21f07 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -745,6 +745,13 @@ struct transport *prepare_transport(struct remote *remote)
return transport;
 }
 
+static void backfill_tags(struct transport *transport, struct ref *ref_map)
+{
+   transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
+   transport_set_option(transport, TRANS_OPT_DEPTH, "0");
+   fetch_refs(transport, ref_map);
+}
+
 static int do_fetch(struct transport *transport,
struct refspec *refs, int ref_count)
 {
@@ -828,11 +835,8 @@ static int do_fetch(struct transport *transport,
struct ref **tail = &ref_map;
ref_map = NULL;
find_non_local_tags(transport, &ref_map, &tail);
-   if (ref_map) {
-   transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 
NULL);
-   transport_set_option(transport, TRANS_OPT_DEPTH, "0");
-   fetch_refs(transport, ref_map);
-   }
+   if (ref_map)
+   backfill_tags(transport, ref_map);
free_refs(ref_map);
}
 
-- 
1.8.4-rc1-210-gf6d87e2

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

[PATCH 5/5] fetch: work around "transport-take-over" hack

[Junio C Hamano]

A Git-aware "connect" transport allows the "transport_take_over" to
redirect generic transport requests like fetch(), push_refs() and
get_refs_list() to the native Git transport handling methods.  The
take-over process replaces transport->data with a fake data that
these method implementations understand.

While this hack works OK for a single request, it breaks when the
transport needs to make more than one requests.  transport->data
that used to hold necessary information for the specific helper to
work correctly is destroyed during the take-over process.

One codepath that this matters is "git fetch" in auto-follow mode;
when it does not get all the tags that ought to point at the history
it got (which can be determined by looking at the peeled tags in the
initial advertisement) from the primary transfer, it internally
makes a second request to complete the fetch.  Because "take-over"
hack has already destroyed the data necessary to talk to the
transport helper by the time this happens, the second request cannot
make a request to the helper to make another connection to fetch
these additional tags.

Mark such a transport as "cannot_reuse", and use a separate
transport to perform the backfill fetch in order to work around
this breakage.

Note that this problem does not manifest itself when running t5802,
because our upload-pack gives you all the necessary auto-followed
tags during the primary transfer.  You would need to step through
"git fetch" in a debugger, stop immediately after the primary
transfer finishes and writes these auto-followed tags, remove the
tag references and repack/prune the repository to convince the
"find-non-local-tags" procedure that the primary transfer failed to
give us all the necessary tags, and then let it continue, in order
to trigger the bug in the secondary transfer this patch fixes.

Signed-off-by: Junio C Hamano 
---
 builtin/fetch.c | 13 +
 transport.c |  2 ++
 transport.h |  6 ++
 3 files changed, 21 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 0b21f07..57ab7e4 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,6 +37,7 @@ static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
 static struct transport *gtransport;
+static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 
@@ -97,6 +98,8 @@ static void unlock_pack(void)
 {
if (gtransport)
transport_unlock_pack(gtransport);
+   if (gsecondary)
+   transport_unlock_pack(gsecondary);
 }
 
 static void unlock_pack_on_signal(int signo)
@@ -747,9 +750,19 @@ struct transport *prepare_transport(struct remote *remote)
 
 static void backfill_tags(struct transport *transport, struct ref *ref_map)
 {
+   if (transport->cannot_reuse) {
+   gsecondary = prepare_transport(transport->remote);
+   transport = gsecondary;
+   }
+
transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);
transport_set_option(transport, TRANS_OPT_DEPTH, "0");
fetch_refs(transport, ref_map);
+
+   if (gsecondary) {
+   transport_disconnect(gsecondary);
+   gsecondary = NULL;
+   }
 }
 
 static int do_fetch(struct transport *transport,
diff --git a/transport.c b/transport.c
index e15db98..de25588 100644
--- a/transport.c
+++ b/transport.c
@@ -875,6 +875,8 @@ void transport_take_over(struct transport *transport,
transport->push_refs = git_transport_push;
transport->disconnect = disconnect_git;
transport->smart_options = &(data->options);
+
+   transport->cannot_reuse = 1;
 }
 
 static int is_local(const char *url)
diff --git a/transport.h b/transport.h
index ea70ea7..96e0ede 100644
--- a/transport.h
+++ b/transport.h
@@ -27,6 +27,12 @@ struct transport {
 */
unsigned got_remote_refs : 1;
 
+   /*
+* Transports that call take-over destroys the data specific to
+* the transport type while doing so, and cannot be reused.
+*/
+   unsigned cannot_reuse : 1;
+
/**
 * Returns 0 if successful, positive if the option is not
 * recognized or is inapplicable, and negative if the option
-- 
1.8.4-rc1-210-gf6d87e2

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

[PATCH 3/5] fetch: refactor code that prepares a transport

[Junio C Hamano]

Make a helper function prepare_transport() that returns a transport
to talk to a given remote.

The set_option() helper that used to always affect the file-scope
global "gtransport" now takes a transport as its parameter.

Signed-off-by: Junio C Hamano 
---
 builtin/fetch.c | 46 ++
 1 file changed, 26 insertions(+), 20 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 636b47f..39a3fc8 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -720,6 +720,31 @@ static int truncate_fetch_head(void)
return 0;
 }
 
+static void set_option(struct transport *transport, const char *name, const 
char *value)
+{
+   int r = transport_set_option(transport, name, value);
+   if (r < 0)
+   die(_("Option \"%s\" value \"%s\" is not valid for %s"),
+   name, value, transport->url);
+   if (r > 0)
+   warning(_("Option \"%s\" is ignored for %s\n"),
+   name, transport->url);
+}
+
+struct transport *prepare_transport(struct remote *remote)
+{
+   struct transport *transport;
+   transport = transport_get(remote, NULL);
+   transport_set_verbosity(transport, verbosity, progress);
+   if (upload_pack)
+   set_option(transport, TRANS_OPT_UPLOADPACK, upload_pack);
+   if (keep)
+   set_option(transport, TRANS_OPT_KEEP, "yes");
+   if (depth)
+   set_option(transport, TRANS_OPT_DEPTH, depth);
+   return transport;
+}
+
 static int do_fetch(struct transport *transport,
struct refspec *refs, int ref_count)
 {
@@ -816,17 +841,6 @@ static int do_fetch(struct transport *transport,
return retcode;
 }
 
-static void set_option(const char *name, const char *value)
-{
-   int r = transport_set_option(gtransport, name, value);
-   if (r < 0)
-   die(_("Option \"%s\" value \"%s\" is not valid for %s"),
-   name, value, gtransport->url);
-   if (r > 0)
-   warning(_("Option \"%s\" is ignored for %s\n"),
-   name, gtransport->url);
-}
-
 static int get_one_remote_for_fetch(struct remote *remote, void *priv)
 {
struct string_list *list = priv;
@@ -949,15 +963,7 @@ static int fetch_one(struct remote *remote, int argc, 
const char **argv)
die(_("No remote repository specified.  Please, specify either 
a URL or a\n"
"remote name from which new revisions should be fetched."));
 
-   gtransport = transport_get(remote, NULL);
-   transport_set_verbosity(gtransport, verbosity, progress);
-   if (upload_pack)
-   set_option(TRANS_OPT_UPLOADPACK, upload_pack);
-   if (keep)
-   set_option(TRANS_OPT_KEEP, "yes");
-   if (depth)
-   set_option(TRANS_OPT_DEPTH, depth);
-
+   gtransport = prepare_transport(remote);
if (argc > 0) {
int j = 0;
refs = xcalloc(argc + 1, sizeof(const char *));
-- 
1.8.4-rc1-210-gf6d87e2

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

[PATCH 0/5] work around "transport-take-over" hack

[Junio C Hamano]

"git fetch" sometimes needs to make a real request to the transport
after a single "fetch_refs" request, in order to follow tags that
the other end should have sent as part of the primary transfer with
"follow-tags" request.

However, a transport that defines "connect" has a gross hack that
destroys its internal state to render it unusable after processing a
single request, breaking this.

This is my attempt to work it around.  I am not too proud of it, but
after trying two other approaches to fix it closer to the real cause
(i.e. the implementation of Git-aware transport helper) and seeing
that both of them ended up being even less appealing and not worth
posting, I think this is probably the best fix to the issue.

Junio C Hamano (5):
  t5802: add test for connect helper
  fetch: rename file-scope global "transport" to "gtransport"
  fetch: refactor code that prepares a transport
  fetch: refactor code that fetches leftover tags
  fetch: work around "transport-take-over" hack

 builtin/fetch.c   | 85 ++-
 t/t5802-connect-helper.sh | 72 +++
 transport.c   |  2 ++
 transport.h   |  6 
 4 files changed, 134 insertions(+), 31 deletions(-)
 create mode 100755 t/t5802-connect-helper.sh

-- 
1.8.4-rc1-210-gf6d87e2

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

[PATCH 1/5] t5802: add test for connect helper

[Junio C Hamano]

This is an attempt to reproduce a problem reported for a third-party
custom "connect" remote helper.  The conjecture is that sometimes
"git fetch" wants to make two connections (one for the primary
transfer with 'follow-tags' option set, and then after noticing that
some tags are not packed because the primary transfer did not have
to send any commit that is pointed by them, another to explicitly
ask for the missing tags), and their "connect" helper is not called
in the second request, breaking the "fetch" as a whole.

Unfortunately this test script does not trigger the alleged failure
and happily passes when talking to upload-pack from git-core (see
patch 5/5 for details).

Signed-off-by: Junio C Hamano 
---
 t/t5802-connect-helper.sh | 72 +++
 1 file changed, 72 insertions(+)
 create mode 100755 t/t5802-connect-helper.sh

diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
new file mode 100755
index 000..878faf2
--- /dev/null
+++ b/t/t5802-connect-helper.sh
@@ -0,0 +1,72 @@
+#!/bin/sh
+
+test_description='ext::cmd remote "connect" helper'
+. ./test-lib.sh
+
+test_expect_success setup '
+   test_tick &&
+   git commit --allow-empty -m initial &&
+   test_tick &&
+   git commit --allow-empty -m second &&
+   test_tick &&
+   git commit --allow-empty -m third &&
+   test_tick &&
+   git tag -a -m "tip three" three &&
+
+   test_tick &&
+   git commit --allow-empty -m fourth
+'
+
+test_expect_success clone '
+   cmd=$(echo "echo >&2 ext::sh invoked && %S .." | sed -e "s/ /% /g") &&
+   git clone "ext::sh -c %S% ." dst &&
+   git for-each-ref refs/heads/ refs/tags/ >expect &&
+   (
+   cd dst &&
+   git config remote.origin.url "ext::sh -c $cmd" &&
+   git for-each-ref refs/heads/ refs/tags/
+   ) >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'update following tag' '
+   test_tick &&
+   git commit --allow-empty -m fifth &&
+   test_tick &&
+   git tag -a -m "tip five" five &&
+   git for-each-ref refs/heads/ refs/tags/ >expect &&
+   (
+   cd dst &&
+   git pull &&
+   git for-each-ref refs/heads/ refs/tags/ >../actual
+   ) &&
+   test_cmp expect actual
+'
+
+test_expect_success 'update backfilled tag' '
+   test_tick &&
+   git commit --allow-empty -m sixth &&
+   test_tick &&
+   git tag -a -m "tip two" two three^1 &&
+   git for-each-ref refs/heads/ refs/tags/ >expect &&
+   (
+   cd dst &&
+   git pull &&
+   git for-each-ref refs/heads/ refs/tags/ >../actual
+   ) &&
+   test_cmp expect actual
+'
+
+test_expect_success 'update backfilled tag without primary transfer' '
+   test_tick &&
+   git tag -a -m "tip one " one two^1 &&
+   git for-each-ref refs/heads/ refs/tags/ >expect &&
+   (
+   cd dst &&
+   git pull &&
+   git for-each-ref refs/heads/ refs/tags/ >../actual
+   ) &&
+   test_cmp expect actual
+'
+
+test_done
-- 
1.8.4-rc1-210-gf6d87e2

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

gitweb, How to script alias to a directory? for cloning over http

[Tyrone Lucero]

Hello, I need some guide that can  explain me the following:

I wish to know the correct rule to make work cloning over http with my
configuration, without taking all the web server to server only as
github

I setup  gitweb to it can show in a directory
by example, localhost/gitweb or 192.168.1.20/gitweb ; and works fine,
including rewrite rule



Config file under conf.d from apache config files:


Alias /gitweb /usr/share/gitweb

SetEnv GITWEB_CONFIG /etc/gitweb.conf
SetEnv GIT_PROJECT_ROOT /var/git
SetEnv GIT_HTTP_EXPORT_ALL


  Options ExecCGI FollowSymLinks Indexes
  AddHandler cgi-script .cgi
  Allow from all
  Order allow,deny
  DirectoryIndex index.cgi
  # Pretty gitweb URLs need rewrite engine on an enabled
  RewriteEngine on
  # rule condition indicates get filenames
  RewriteCond %{REQUEST_FILENAME} !-f
  # rule condition indicates get listing directories
  RewriteCond %{REQUEST_FILENAME} !-d
  # rule condition to show pretty short urls
  RewriteRule ^.* /gitweb/gitweb.cgi/$0 [L,PT]


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

Re: Remove old forgotten command: whatchanged

[Stefan Beller]

On 08/07/2013 07:50 PM, Junio C Hamano wrote:
> Stefan Beller  writes:
> 
>> I'd deprecate it first for a year or such and remove it then.
>> In the meantime we could implement already remove the code
>> and change it to:
>>
>> + int cmd_whatchanged(int argc, const char **argv, const char *prefix)
>> + {
>> +return cmd_log(argc, argv, prefix)
>> + }
>>
>> Also we should make sure everything git whatchanged can do,
>> can easily be done with git log .
> 
> That's even worse than "deprecating".
> 
> Your first step already changes the behaviour for people who really
> use the command, without telling them _why_ the behaviour has
> suddenly changed at all, while not helping *ANYBODY*.
> 
> The only thing it does is to scratch an irrelevant itch by people
> who peek the codebase and find an old command whose existence does
> not even hurt them.  They may have too much time on their hand, but
> that is not an excuse to waste other people's time by adding extra
> makework on our plate, or changing the behaviour for people who use
> the command without explanation.
> 
> Feeling irritated somewhat...
> 

Well if we make sure the whatchanged command can easily be reproduced
with the log command, we could add the missing parameters to it, hence
no change for the user. (git whatchanged == git log --raw --no-merges or
git log --wc [to be done yet]).

So I did not mean to introduce a change for users!

But in general I like the idea to *remove* lines of code.

Stefan





signature.asc
Description: OpenPGP digital signature

Re: Repo with only one file

[shawn wilson]

On Wed, Aug 7, 2013 at 6:43 AM, Johannes Sixt  wrote:
> Am 8/7/2013 8:24, schrieb shawn wilson:> ... create a repo for one of
>> these scripts and I'd like to keep the commit history.
>>
>> Ok, so:
>> % find -type f ! -iname "webban.pl" | while read f; do git
>> filter-branch -f --index-filter "git rm --cached --ignore-unmatch $f"
>> HEAD ; done
>>
>> Which basically did it. But, I've got this one commit that seems to be
>> orphaned - it doesn't change any files.
>
> Try this:
>
>   git filter-branch HEAD -- webban.pl
>

 % git filter-branch HEAD -- webban.pl
Cannot create a new backup.
A previous backup already exists in refs/original/
Force overwriting the backup with -f
 % git filter-branch -f HEAD -- webban.pl
Rewrite 1e04b18c256c996312f167be808733bcc755f1e3 (9/9)
WARNING: Ref 'refs/heads/master' is unchanged
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Remove old forgotten command: whatchanged

[Kyle J. McKay]

On Aug 7, 2013, at 11:31, John Keeping wrote:

On Wed, Aug 07, 2013 at 11:01:57AM -0700, Kyle J. McKay wrote:

On Aug 7, 2013, at 09:00, Ramkumar Ramachandra wrote:

Hi,

This is the difference between whatchanged and log:

diff --git a/whatchanged b/log
index fa1b223..004d9aa 100644
--- a/tmp/whatchanged
+++ b/tmp/log
@@ -1,4 +1,4 @@
-int cmd_whatchanged(int argc, const char **argv, const char  
*prefix)

+int cmd_log(int argc, const char **argv, const char *prefix)
{
  struct rev_info rev;
  struct setup_revision_opt opt;
@@ -7,13 +7,10 @@ int cmd_whatchanged(int argc, const char **argv,
const char *prefix)
  git_config(git_log_config, NULL);

  init_revisions(&rev, prefix);
-   rev.diff = 1;
-   rev.simplify_history = 0;
+   rev.always_show_header = 1;
  memset(&opt, 0, sizeof(opt));
  opt.def = "HEAD";
  opt.revarg_opt = REVARG_COMMITTISH;
  cmd_log_init(argc, argv, prefix, &rev, &opt);
-   if (!rev.diffopt.output_format)
-   rev.diffopt.output_format = DIFF_FORMAT_RAW;
  return cmd_log_walk(&rev);
}

Should we remove it?


I use it all the time.  Is there some log option to get exactly the
same output?  It doesn't appear that there is.  The closest looks  
like

"log --name-status" but it omits the modes and hash values.


Is it not identical to "log --raw --no-merges"?

A quick test says "mostly", but whatchanged doesn't show empty commits
whereas log does seem to; e.g. in git.git:

   diff -u <(git whatchanged) <(git log --raw --no-merges)


That's probably close enough, but if that's all "whatchanged" does is  
basically be an alias for "log --raw --no-merges" that skips commits  
with no changes then the help for whatchanged ought to mention that.


I still think "git whatchanged" is a lot clearer than "git log --raw -- 
no-merges" 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: Remove old forgotten command: whatchanged

[John Keeping]

On Wed, Aug 07, 2013 at 11:01:57AM -0700, Kyle J. McKay wrote:
> On Aug 7, 2013, at 09:00, Ramkumar Ramachandra wrote:
> > Hi,
> >
> > This is the difference between whatchanged and log:
> >
> > diff --git a/whatchanged b/log
> > index fa1b223..004d9aa 100644
> > --- a/tmp/whatchanged
> > +++ b/tmp/log
> > @@ -1,4 +1,4 @@
> > -int cmd_whatchanged(int argc, const char **argv, const char *prefix)
> > +int cmd_log(int argc, const char **argv, const char *prefix)
> > {
> >struct rev_info rev;
> >struct setup_revision_opt opt;
> > @@ -7,13 +7,10 @@ int cmd_whatchanged(int argc, const char **argv,
> > const char *prefix)
> >git_config(git_log_config, NULL);
> >
> >init_revisions(&rev, prefix);
> > -   rev.diff = 1;
> > -   rev.simplify_history = 0;
> > +   rev.always_show_header = 1;
> >memset(&opt, 0, sizeof(opt));
> >opt.def = "HEAD";
> >opt.revarg_opt = REVARG_COMMITTISH;
> >cmd_log_init(argc, argv, prefix, &rev, &opt);
> > -   if (!rev.diffopt.output_format)
> > -   rev.diffopt.output_format = DIFF_FORMAT_RAW;
> >return cmd_log_walk(&rev);
> > }
> >
> > Should we remove it?
> 
> I use it all the time.  Is there some log option to get exactly the  
> same output?  It doesn't appear that there is.  The closest looks like  
> "log --name-status" but it omits the modes and hash values.

Is it not identical to "log --raw --no-merges"?

A quick test says "mostly", but whatchanged doesn't show empty commits
whereas log does seem to; e.g. in git.git:

diff -u <(git whatchanged) <(git log --raw --no-merges)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Fredrik Gustafsson]

On Tue, Aug 06, 2013 at 02:11:56PM -0700, Junio C Hamano wrote:
> Thanks, will replace the top two commits and queue.  Looks like we
> are getting ready for 'next'?

I'm a bit curious about if we should move towards a reentrent libgit
(which would for example make multithreading easier) or not.

If so, I suggest that this patch only use die() in builtin/. However I
know that there's a lot of die() all over libgit today, I'm curious
about what direction we're heading.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iv...@iveqy.com
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Remove old forgotten command: whatchanged

[Kyle J. McKay]

On Aug 7, 2013, at 09:00, Ramkumar Ramachandra wrote:

Hi,

This is the difference between whatchanged and log:

diff --git a/whatchanged b/log
index fa1b223..004d9aa 100644
--- a/tmp/whatchanged
+++ b/tmp/log
@@ -1,4 +1,4 @@
-int cmd_whatchanged(int argc, const char **argv, const char *prefix)
+int cmd_log(int argc, const char **argv, const char *prefix)
{
   struct rev_info rev;
   struct setup_revision_opt opt;
@@ -7,13 +7,10 @@ int cmd_whatchanged(int argc, const char **argv,
const char *prefix)
   git_config(git_log_config, NULL);

   init_revisions(&rev, prefix);
-   rev.diff = 1;
-   rev.simplify_history = 0;
+   rev.always_show_header = 1;
   memset(&opt, 0, sizeof(opt));
   opt.def = "HEAD";
   opt.revarg_opt = REVARG_COMMITTISH;
   cmd_log_init(argc, argv, prefix, &rev, &opt);
-   if (!rev.diffopt.output_format)
-   rev.diffopt.output_format = DIFF_FORMAT_RAW;
   return cmd_log_walk(&rev);
}

Should we remove it?


I use it all the time.  Is there some log option to get exactly the  
same output?  It doesn't appear that there is.  The closest looks like  
"log --name-status" but it omits the modes and hash values.

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

Re: Remove old forgotten command: whatchanged

[Junio C Hamano]

Stefan Beller  writes:

> I'd deprecate it first for a year or such and remove it then.
> In the meantime we could implement already remove the code
> and change it to:
>
> + int cmd_whatchanged(int argc, const char **argv, const char *prefix)
> + {
> + return cmd_log(argc, argv, prefix)
> + }
>
> Also we should make sure everything git whatchanged can do,
> can easily be done with git log .

That's even worse than "deprecating".

Your first step already changes the behaviour for people who really
use the command, without telling them _why_ the behaviour has
suddenly changed at all, while not helping *ANYBODY*.

The only thing it does is to scratch an irrelevant itch by people
who peek the codebase and find an old command whose existence does
not even hurt them.  They may have too much time on their hand, but
that is not an excuse to waste other people's time by adding extra
makework on our plate, or changing the behaviour for people who use
the command without explanation.

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

Re: [PATCH v4 5/5] rm: delete .gitmodules entry of submodules removed from the work tree

[Jens Lehmann]

Am 06.08.2013 23:11, schrieb Junio C Hamano:
> Thanks, will replace the top two commits and queue.  Looks like we
> are getting ready for 'next'?

I hope so, I'm not aware of any open issues.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Remove old forgotten command: whatchanged

[Stefan Beller]

On 08/07/2013 06:00 PM, Ramkumar Ramachandra wrote:
> Hi,
> 
> This is the difference between whatchanged and log:
> 
> diff --git a/whatchanged b/log
> index fa1b223..004d9aa 100644
> --- a/tmp/whatchanged
> +++ b/tmp/log
> @@ -1,4 +1,4 @@
> -int cmd_whatchanged(int argc, const char **argv, const char *prefix)
> +int cmd_log(int argc, const char **argv, const char *prefix)
>  {
> struct rev_info rev;
> struct setup_revision_opt opt;
> @@ -7,13 +7,10 @@ int cmd_whatchanged(int argc, const char **argv,
> const char *prefix)
> git_config(git_log_config, NULL);
> 
> init_revisions(&rev, prefix);
> -   rev.diff = 1;
> -   rev.simplify_history = 0;
> +   rev.always_show_header = 1;
> memset(&opt, 0, sizeof(opt));
> opt.def = "HEAD";
> opt.revarg_opt = REVARG_COMMITTISH;
> cmd_log_init(argc, argv, prefix, &rev, &opt);
> -   if (!rev.diffopt.output_format)
> -   rev.diffopt.output_format = DIFF_FORMAT_RAW;
> return cmd_log_walk(&rev);
>  }
> 
> Should we remove it?

I'd deprecate it first for a year or such and remove it then.
In the meantime we could implement already remove the code
and change it to:

+ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
+ {
+   return cmd_log(argc, argv, prefix)
+ }

Also we should make sure everything git whatchanged can do,
can easily be done with git log .

Stefan



signature.asc
Description: OpenPGP digital signature

Re: [PATCH 1/4] Build in git-repack

[Stefan Beller]

On 08/07/2013 05:48 PM, Junio C Hamano wrote:
> Matthieu Moy  writes:
>>
>> seems overkill to me: why don't you just let cmd_repack call
>> update_server_info(0)?
> 
> My feeling exactly.  I would rather see a patch that does not touch
> pack-objects at all, and use run_command() interface to spawn it.
> Once we do have to pack, the necessary processing cycle will dwarf
> the fork/exec latency anyway, no?
> 

Thanks for clarification. That was my initial idea as well, 
to not touch the pack-objects logic. 

However Duy send that patch (basically as is, 
I just made it apply again), 
and I guessed that I'd get to results
faster with an already existing approach.

I will reconsider and try to remove the additional logic from 
pack-objects again (so it will not get touched) and move it to the
repack itself. It is just a way to understand, 'what needs to be done'.

Stefan



signature.asc
Description: OpenPGP digital signature

Re: [RFC] status: show tracking branch even no difference

[Jiang Xin]

2013/8/7 Matthieu Moy :
> Jiang Xin  writes:
>
>> With this patch, "git status" will report relationship between current
>> branch and its upstream counterpart even if there is no difference.
>>
>> $ git status
>> # On branch master
>> # Your branch is identical to its tracking branch: 'origin/master'.
>
> Why not, but we try to say "remote-tracking branch" instead of just
> "tracking". Adding "remote-" in your wording may make the line a bit
> long, but it may be sufficient to say
>
> # Your branch is identical to 'origin/master'

That's better. Thanks.

>
> That's consistant with other messages like
>
> # Your branch is ahead of '%s' by %d commits
>
> (And this would deserve a test)

Will add some test cases in t6040 if this patch has value.

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

Remove old forgotten command: whatchanged

[Ramkumar Ramachandra]

Hi,

This is the difference between whatchanged and log:

diff --git a/whatchanged b/log
index fa1b223..004d9aa 100644
--- a/tmp/whatchanged
+++ b/tmp/log
@@ -1,4 +1,4 @@
-int cmd_whatchanged(int argc, const char **argv, const char *prefix)
+int cmd_log(int argc, const char **argv, const char *prefix)
 {
struct rev_info rev;
struct setup_revision_opt opt;
@@ -7,13 +7,10 @@ int cmd_whatchanged(int argc, const char **argv,
const char *prefix)
git_config(git_log_config, NULL);

init_revisions(&rev, prefix);
-   rev.diff = 1;
-   rev.simplify_history = 0;
+   rev.always_show_header = 1;
memset(&opt, 0, sizeof(opt));
opt.def = "HEAD";
opt.revarg_opt = REVARG_COMMITTISH;
cmd_log_init(argc, argv, prefix, &rev, &opt);
-   if (!rev.diffopt.output_format)
-   rev.diffopt.output_format = DIFF_FORMAT_RAW;
return cmd_log_walk(&rev);
 }

Should we remove it?

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

"git log --follow filename" (with diff.renameLimit=0) not working after subtree add

[Diogo de Campos]

Problem:

"git log --follow filename" is not working for files after a big move.


Scenario:

Just performed a "git subtree add" where big projects were moved into
another repository, and I'm not able to view history for a single
file.

Sample output:
Output after subtree add (no output)
> git log --follow --oneline file.py

Output from same command in old repository (the one that was merged
with subtree, this is the output i expect to continue having atfer a
merge)
> git log --follow --oneline file.py
cefa32b Convert doc strings to Sphinx
748b2dd Added target for C++ tests
1f2b52e Using default git_source_server for everything after moving to Stash
... [more commits]


A couple of notes:
- Made sure I used --follow
- Using options diff.renameLimit=0 and diff.renames=true
- Tried options -C and -M (even though there were no changes, should
be a 100% rename match)
- "git subtree add" was made without --squash to keep history
- "git blame file.py" works fine, detects all changes and rename history
- Commits added after the merge are tracked, but log stops at merge
- Tested with git 1.8.3.4
- This guy in StackOverflow seems to have the same problem:
http://stackoverflow.com/questions/4393527/why-might-git-log-not-show-history-for-a-moved-file-and-what-can-i-do-about-it

Relevant output for "git config -l" in repository:
core.symlinks=false
core.autocrlf=false
color.diff=auto
color.status=auto
color.branch=auto
color.interactive=true
pack.packsizelimit=2g
help.format=html
http.sslcainfo=/bin/curl-ca-bundle.crt
sendemail.smtpserver=/bin/msmtp.exe
diff.astextplain.textconv=astextplain
rebase.autosquash=true
core.packedgitlimit=128m
diff.renamelimit=0
diff.renames=true
core.repositoryformatversion=0
core.filemode=false
core.bare=false
core.logallrefupdates=true
core.symlinks=false
core.ignorecase=true
core.hidedotfiles=dotGitOnly

--

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

Re: [RFC] status: show tracking branch even no difference

[Matthieu Moy]

Jiang Xin  writes:

> With this patch, "git status" will report relationship between current
> branch and its upstream counterpart even if there is no difference.
>
> $ git status
> # On branch master
> # Your branch is identical to its tracking branch: 'origin/master'.

Why not, but we try to say "remote-tracking branch" instead of just
"tracking". Adding "remote-" in your wording may make the line a bit
long, but it may be sufficient to say

# Your branch is identical to 'origin/master'

That's consistant with other messages like

# Your branch is ahead of '%s' by %d commits

(And this would deserve a test)

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Build in git-repack

[Junio C Hamano]

Matthieu Moy  writes:

> [ It's cool you're working on this, I'd really like a git-repack in C.
>   That would fix this
>   http://thread.gmane.org/gmane.comp.version-control.git/226458 ]
>
> Stefan Beller  writes:
>
>> From: Nguyễn Thái Ngọc Duy 
>>
>> pack-objects learns a few more options to take over what's been done
>> by git-repack.sh. cmd_repack() becomes a wrapper around
>> cmd_pack_objects().
>
> I think the patch would read easier if these were split into two
> patches: one doing the real stuff in pack-objects, and then getting rid
> of git-repack.sh to replace it with a trivial built-in.
>
> Actually, I'm wondering why pack-objects requires so much changes.
> git-repack.sh was already a relatively small wrapper around
> pack-objects, and did not need the new options you add, so why are they
> needed? In particular adding the new --update-info option that just does
>
>> +if (repack_flags & REPACK_UPDATE_INFO)
>> +update_server_info(0);
>
> seems overkill to me: why don't you just let cmd_repack call
> update_server_info(0)?

My feeling exactly.  I would rather see a patch that does not touch
pack-objects at all, and use run_command() interface to spawn it.
Once we do have to pack, the necessary processing cycle will dwarf
the fork/exec latency anyway, no?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] status: show tracking branch even no difference

[Jiang Xin]

If the current branch has an upstream branch, and there are differences
between the current branch and its upstream, some commands (such as
"git status", "git status -bs", and "git checkout") will report their
relationship. E.g.

$ git status
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#   (use "git push" to publish your local commits)
#
...

$ git status -bs
## master...origin/master [ahead 1]
...

$ git checkout master
Already on 'master'
Your branch is ahead of 'origin/master' by 1 commit.
  (use "git push" to publish your local commits)

But if there is no difference between the current branch and its
upstream, the relationship will not be reported, and it's hard to
tell whether the current branch has a tracking branch or not. And
what's worse, when the 'push.default' config variable is set to
`matching`, it's hard to tell whether current branch is pushed out
or not [1].

With this patch, "git status" will report relationship between current
branch and its upstream counterpart even if there is no difference.

$ git status
# On branch master
# Your branch is identical to its tracking branch: 'origin/master'.
#
...

$ git status -bs
## master...origin/master
...

$ git checkout master
Already on 'master'
Your branch is identical to its tracking branch: 'origin/master'.

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

Signed-off-by: Jiang Xin 
---
 remote.c| 22 --
 wt-status.c | 13 ++---
 2 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/remote.c b/remote.c
index 2433467..8d6f278 100644
--- a/remote.c
+++ b/remote.c
@@ -1740,6 +1740,10 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
const char *rev_argv[10], *base;
int rev_argc;
 
+   /* Set both num_theirs and num_ours as undetermined. */
+   *num_theirs = -1;
+   *num_ours = -1;
+
/*
 * Nothing to report unless we are marked to build on top of
 * somebody else.
@@ -1758,14 +1762,16 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
theirs = lookup_commit_reference(sha1);
if (!theirs)
return 0;
+   *num_theirs = 0;
 
if (read_ref(branch->refname, sha1))
return 0;
ours = lookup_commit_reference(sha1);
if (!ours)
return 0;
+   *num_ours = 0;
 
-   /* are we the same? */
+   /* are we the same? both num_theirs and num_ours are set to 0. */
if (theirs == ours)
return 0;
 
@@ -1786,8 +1792,6 @@ int stat_tracking_info(struct branch *branch, int 
*num_ours, int *num_theirs)
prepare_revision_walk(&revs);
 
/* ... and count the commits on each side. */
-   *num_ours = 0;
-   *num_theirs = 0;
while (1) {
struct commit *c = get_revision(&revs);
if (!c)
@@ -1812,12 +1816,18 @@ int format_tracking_info(struct branch *branch, struct 
strbuf *sb)
int num_ours, num_theirs;
const char *base;
 
-   if (!stat_tracking_info(branch, &num_ours, &num_theirs))
-   return 0;
+   if (!stat_tracking_info(branch, &num_ours, &num_theirs)) {
+   if (num_ours || num_theirs)
+   return 0;
+   }
 
base = branch->merge[0]->dst;
base = shorten_unambiguous_ref(base, 0);
-   if (!num_theirs) {
+   if (!num_ours && !num_theirs) {
+   strbuf_addf(sb,
+   _("Your branch is identical to its tracking branch: 
'%s'.\n"),
+   base);
+   } else if (!num_theirs) {
strbuf_addf(sb,
Q_("Your branch is ahead of '%s' by %d commit.\n",
   "Your branch is ahead of '%s' by %d commits.\n",
diff --git a/wt-status.c b/wt-status.c
index ff4b324..56f3c19 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1381,9 +1381,11 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
if (s->is_initial)
color_fprintf(s->fp, header_color, _("Initial commit on "));
if (!stat_tracking_info(branch, &num_ours, &num_theirs)) {
-   color_fprintf(s->fp, branch_color_local, "%s", branch_name);
-   fputc(s->null_termination ? '\0' : '\n', s->fp);
-   return;
+   if (num_ours || num_theirs) {
+   color_fprintf(s->fp, branch_color_local, "%s", 
branch_name);
+   fputc(s->null_termination ? '\0' : '\n', s->fp);
+   return;
+   }
}
 
base = branch->merge[0]->dst;
@@ -1392,6 +1394,11 @@ static void wt_shortstatus_print_tracking(struct 
wt_status *s)
color_fprintf(s->fp, header_color, "...");
color_fprintf(s->fp, branch_color_remote, "%s", base);

Re: [RFH/PATCH 4/4] OPT__FORCE(): clarify its expected use by using OPT_COUNTUP

[Junio C Hamano]

Stefan Beller  writes:

> On 08/07/2013 01:31 AM, Junio C Hamano wrote:
>> The parseopt parsing for OPT__FORCE() is implemented in terms of
>> OPT_BOOLEAN() and users of it can take advantage of the "counting
>> up" behaviour to implement increasing levels of forcefulness by
>> differentiating "git cmd -f" and "git cmd -f -f".
>> 
>> Clarify this by explicitly using OPT_COUNTUP() instead.
>> 
>> Signed-off-by: Junio C Hamano 
>> ---
>> 
>>  * This _should_ be done with a similar audit of existing callers,
>>but I ran out of concentration.
>> 
>>  parse-options.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/parse-options.h b/parse-options.h
>> index 78f52c2..1eeb0d9 100644
>> --- a/parse-options.h
>> +++ b/parse-options.h
>> @@ -238,7 +238,7 @@ extern int parse_opt_noop_cb(const struct option *, 
>> const char *, int);
>>  { OPTION_CALLBACK, 'q', "quiet", (var), NULL, N_("be more quiet"), \
>>PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }
>>  #define OPT__DRY_RUN(var, h)  OPT_BOOL('n', "dry-run", (var), (h))
>> -#define OPT__FORCE(var, h)OPT_BOOLEAN('f', "force",   (var), (h))
>> +#define OPT__FORCE(var, h)OPT_COUNTUP('f', "force",   (var), (h))
>>  #define OPT__ABBREV(var)  \
>>  { OPTION_CALLBACK, 0, "abbrev", (var), N_("n"), \
>>N_("use  digits to display SHA-1s"),   \
>> 
>
> We need the COUNTUP, because in builtin/clean.c we have
>   OPT__FORCE(&force, N_("force")),
>   ...
>   if (force > 1)
>   rm_flags = 0;

Good that I marked it as RFH ;-)  Thanks.

> So a OPT_BOOL definitely doesn't cut it.
> Now that I started reviewing the OPT_FORCE parts, I realize
> there is still an error in the patch, which needed correction.
> (branch, commit, name-rev: ease up boolean conditions):
>
> - if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
> !!unset_upstream > 1)
> + if (force_create + list + unset_upstream +
> + !!delete + !!rename + !!new_upstream > 1)
>   usage_with_options(builtin_branch_usage, options);
>
> force_create is set via OPT_FORCE as well, so we cannot remove the !! before 
> the force_create,
> hence we'd only remove it from list and unset_upstream, which are set by 
> OPT_BOOL.

Good.  Will replace.  Thanks.

> -- 8< --
> From: Stefan Beller 
> Date: Wed, 7 Aug 2013 09:32:25 +0200
> Subject: [PATCH] branch, commit, name-rev: ease up boolean conditions
>
> Now that the variables are set by OPT_BOOL, which makes sure
> to have the values being 0 or 1 after parsing, we do not need
> the double negation to map any other value to 1 for integer
> variables.
>
> Signed-off-by: Stefan Beller 
> Signed-off-by: Junio C Hamano 
> ---
>  builtin/branch.c   | 3 ++-
>  builtin/commit.c   | 2 +-
>  builtin/name-rev.c | 2 +-
>  3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 4daed0b..0903763 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -872,7 +872,8 @@ int cmd_branch(int argc, const char **argv, const char 
> *prefix)
>   if (with_commit || merge_filter != NO_FILTER)
>   list = 1;
>  
> - if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
> !!unset_upstream > 1)
> + if (!!delete + !!rename + !!force_create + !!new_upstream +
> + list + unset_upstream > 1)
>   usage_with_options(builtin_branch_usage, options);
>  
>   if (abbrev == -1)
> diff --git a/builtin/commit.c b/builtin/commit.c
> index c20426b..b0f86c8 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1072,7 +1072,7 @@ static int parse_and_validate_options(int argc, const 
> char *argv[],
>   if (patch_interactive)
>   interactive = 1;
>  
> - if (!!also + !!only + !!all + !!interactive > 1)
> + if (also + only + all + interactive > 1)
>   die(_("Only one of --include/--only/--all/--interactive/--patch 
> can be used."));
>   if (argc == 0 && (also || (only && !amend)))
>   die(_("No paths with --include/--only does not make sense."));
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index a908a34..20fcf8c 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -331,7 +331,7 @@ int cmd_name_rev(int argc, const char **argv, const char 
> *prefix)
>  
>   git_config(git_default_config, NULL);
>   argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
> - if (!!all + !!transform_stdin + !!argc > 1) {
> + if (all + transform_stdin + !!argc > 1) {
>   error("Specify either a list, or --all, not both!");
>   usage_with_options(name_rev_usage, opts);
>   }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] Build in git-repack

[Matthieu Moy]

[ It's cool you're working on this, I'd really like a git-repack in C.
  That would fix this
  http://thread.gmane.org/gmane.comp.version-control.git/226458 ]

Stefan Beller  writes:

> From: Nguyễn Thái Ngọc Duy 
>
> pack-objects learns a few more options to take over what's been done
> by git-repack.sh. cmd_repack() becomes a wrapper around
> cmd_pack_objects().

I think the patch would read easier if these were split into two
patches: one doing the real stuff in pack-objects, and then getting rid
of git-repack.sh to replace it with a trivial built-in.

Actually, I'm wondering why pack-objects requires so much changes.
git-repack.sh was already a relatively small wrapper around
pack-objects, and did not need the new options you add, so why are they
needed? In particular adding the new --update-info option that just does

> + if (repack_flags & REPACK_UPDATE_INFO)
> + update_server_info(0);

seems overkill to me: why don't you just let cmd_repack call
update_server_info(0)?

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

[PATCH 0/4] Re: Rewriting git-repack.sh in C

[Stefan Beller]

Hi Duy,

I applied your patch on the current master and added 3 patches, 
so git compiles and the testsuite works without additional breakages.

The functionality is obviously not yet completed as the backup_file
function is still empty. What do you intend that function will do?

Stefan

Nguyễn Thái Ngọc Duy (1):
  Build in git-repack

Stefan Beller (3):
  backup_file dummy function
  pack-objects: do not print usage when repacking
  repack: add unpack-unreachable

 Makefile   |   2 +-
 builtin.h  |   1 +
 builtin/pack-objects.c | 284 -
 bulk-checkin.c |   2 +-
 contrib/examples/git-repack.sh | 194 
 git-repack.sh  | 194 
 git.c  |   1 +
 pack-write.c   |  18 ++-
 pack.h |   4 +-
 9 files changed, 497 insertions(+), 203 deletions(-)
 create mode 100755 contrib/examples/git-repack.sh
 delete mode 100755 git-repack.sh

-- 
1.8.4.rc1.25.g2793d0a

--
To unsubscribe from this list: send the line "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] Build in git-repack

[Stefan Beller]

From: Nguyễn Thái Ngọc Duy 

pack-objects learns a few more options to take over what's been done
by git-repack.sh. cmd_repack() becomes a wrapper around
cmd_pack_objects().
---
 Makefile   |   2 +-
 builtin.h  |   1 +
 builtin/pack-objects.c | 279 -
 bulk-checkin.c |   2 +-
 contrib/examples/git-repack.sh | 194 
 git-repack.sh  | 194 
 git.c  |   1 +
 pack-write.c   |  13 +-
 pack.h |   4 +-
 9 files changed, 488 insertions(+), 202 deletions(-)
 create mode 100755 contrib/examples/git-repack.sh
 delete mode 100755 git-repack.sh

diff --git a/Makefile b/Makefile
index 3588ca1..8bd122b 100644
--- a/Makefile
+++ b/Makefile
@@ -464,7 +464,6 @@ SCRIPT_SH += git-pull.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
-SCRIPT_SH += git-repack.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
 SCRIPT_SH += git-submodule.sh
@@ -596,6 +595,7 @@ BUILT_INS += git-get-tar-commit-id$X
 BUILT_INS += git-init$X
 BUILT_INS += git-merge-subtree$X
 BUILT_INS += git-peek-remote$X
+BUILT_INS += git-repack$X
 BUILT_INS += git-repo-config$X
 BUILT_INS += git-show$X
 BUILT_INS += git-stage$X
diff --git a/builtin.h b/builtin.h
index 8afa2de..b56cf07 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_reflog(int argc, const char **argv, const 
char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
 extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
+extern int cmd_repack(int argc, const char **argv, const char *prefix);
 extern int cmd_repo_config(int argc, const char **argv, const char *prefix);
 extern int cmd_rerere(int argc, const char **argv, const char *prefix);
 extern int cmd_reset(int argc, const char **argv, const char *prefix);
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index f069462..1742ea1 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -18,10 +18,17 @@
 #include "refs.h"
 #include "streaming.h"
 #include "thread-utils.h"
+#include "sigchain.h"
 
 static const char *pack_usage[] = {
N_("git pack-objects --stdout [options...] [< ref-list | < 
object-list]"),
N_("git pack-objects [options...] base-name [< ref-list | < 
object-list]"),
+   N_("git pack-objects --repack [options...]"),
+   NULL
+};
+
+static char const * const repack_usage[] = {
+   N_("git repack [options]"),
NULL
 };
 
@@ -103,6 +110,15 @@ static struct object_entry *locate_object_entry(const 
unsigned char *sha1);
 static uint32_t written, written_delta;
 static uint32_t reused, reused_delta;
 
+#define REPACK_IN_PROGRESS (1 << 0)
+#define REPACK_UPDATE_INFO (1 << 1)
+#define REPACK_ALL_INTO_ONE(1 << 2)
+#define REPACK_REMOVE_REDUNDANT(1 << 3)
+
+static int repack_flags, nr_written_packs;
+static int repack_usedeltabaseoffset;
+static struct string_list written_packs;
+static struct string_list backup_files;
 
 static void *get_delta(struct object_entry *entry)
 {
@@ -792,9 +808,19 @@ static void write_pack_file(void)
snprintf(tmpname, sizeof(tmpname), "%s-", base_name);
finish_tmp_packfile(tmpname, pack_tmp_name,
written_list, nr_written,
-   &pack_idx_opts, sha1);
+   &pack_idx_opts, sha1,
+   repack_flags & REPACK_IN_PROGRESS ?
+   &backup_files : NULL);
free(pack_tmp_name);
-   puts(sha1_to_hex(sha1));
+   if (repack_flags & REPACK_IN_PROGRESS) {
+   int len = strlen(tmpname);
+   char *s = xmalloc(len + 2);
+   memcpy(s, tmpname, len - 4);
+   memcpy(s + len - 4, ".pack", 6);
+   string_list_append(&written_packs, s);
+   nr_written_packs++;
+   } else
+   puts(sha1_to_hex(sha1));
}
 
/* mark written objects as written to previous pack */
@@ -2359,7 +2385,8 @@ static void get_object_list(int ac, const char **av)
save_commit_buffer = 0;
setup_revisions(ac, av, &revs, NULL);
 
-   while (fgets(line, sizeof(line), stdin) != NULL) {
+   while (!(repack_flags & REPACK_IN_PROGRESS) &&
+  fgets(line, sizeof(line), stdin) != NULL) {
int len = strlen(line);
if (

[PATCH 3/4] pack-objects: do not print usage when repacking

[Stefan Beller]

---
 builtin/pack-objects.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 1742ea1..0bd8f3b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2585,7 +2585,7 @@ int cmd_pack_objects(int argc, const char **argv, const 
char *prefix)
base_name = argv[0];
argc--;
}
-   if (pack_to_stdout != !base_name || argc)
+   if (!(repack_flags & REPACK_IN_PROGRESS) && (pack_to_stdout != 
!base_name || argc))
usage_with_options(pack_usage, pack_objects_options);
 
rp_av[rp_ac++] = "pack-objects";
-- 
1.8.4.rc1.25.g2793d0a

--
To unsubscribe from this list: send the line "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] repack: add unpack-unreachable

[Stefan Beller]

---
 builtin/pack-objects.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 0bd8f3b..0fe01fc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2795,6 +2795,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
OPT_BOOL('q', NULL, &quiet, "be quiet"),
OPT_BOOL('l', NULL, &local,
 "pass --local to git-pack-objects"),
+   { OPTION_CALLBACK, 0, "unpack-unreachable", NULL, N_("time"),
+ N_("unpack unreachable objects newer than "),
+ PARSE_OPT_OPTARG, option_parse_unpack_unreachable },
{ OPTION_ARGUMENT, 0, "window", NULL, "n",
 "size of the window used for delta compression", 0 },
{ OPTION_ARGUMENT, 0, "window-memory", NULL, "n",
-- 
1.8.4.rc1.25.g2793d0a

--
To unsubscribe from this list: send the line "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] backup_file dummy function

[Stefan Beller]

---
 pack-write.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/pack-write.c b/pack-write.c
index e6aa7e3..b728ea2 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -344,6 +344,11 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
 }
 
+void backup_file(char *filename)
+{
+
+}
+
 void finish_tmp_packfile(char *name_buffer,
 const char *pack_tmp_name,
 struct pack_idx_entry **written_list,
-- 
1.8.4.rc1.25.g2793d0a

--
To unsubscribe from this list: send the line "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 about Japanese' Documentation

[a a]

# I am sorry. I forget title and signiture.

-
Hello.

I find 2 bug about Japanese'  Documentation .
There are Documentation -> Book 's url. ( Please see the details below).

I want to bug-fix about this misspell.

Do you have Documentation (about Japanese language) on the GitHub?

(1)
http://git-scm.com/book/ja/Git-%E3%81%AE%E5%9F%BA%E6%9C%AC-%E4%BD%9C%E6%A5%AD%E3%81%AE%E3%82%84%E3%82%8A%E7%9B%B4%E3%81%97

(2)
http://git-scm.com/book/ja/Git-%E3%81%AE%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81%E6%A9%9F%E8%83%BD-%E3%83%AA%E3%83%A2%E3%83%BC%E3%83%88%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81


thanks

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

[no subject]

[a a]

Hello.

I find 2 bug about Japanese'  Documentation .
There are Documentation -> Book 's url. ( Please see the details below).

I want to bug-fix about this misspell.

Do you have Documentation (about Japanese language) on the GitHub?

(1)
http://git-scm.com/book/ja/Git-%E3%81%AE%E5%9F%BA%E6%9C%AC-%E4%BD%9C%E6%A5%AD%E3%81%AE%E3%82%84%E3%82%8A%E7%9B%B4%E3%81%97

(2)
http://git-scm.com/book/ja/Git-%E3%81%AE%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81%E6%A9%9F%E8%83%BD-%E3%83%AA%E3%83%A2%E3%83%BC%E3%83%88%E3%83%96%E3%83%A9%E3%83%B3%E3%83%81
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3] git-tag man: when to use lightweight or annotated tags

[Daniele Segato]

On 07/29/2013 08:02 PM, Daniele Segato wrote:

On 07/26/2013 09:36 PM, Jonathan Nieder wrote:

Eventually the description section should probably be tweaked to start
by explaining what the command is actually for. ;-)


Elaborating from this suggestion you gave me I tried to
rewrite/rearrange the description moving things around a little.

Here's what I've come out with, what do you think about it?



DESCRIPTION
---

A tag is a non-mutable reference name (in `refs/tags/`) to an object
(usually a commit).

If one of `-d/-l/-v` options is given the command will delete, list or
verify tags.

If one of `-a`, `-s`, or `-u ` is passed, the command
creates both the reference and a 'tag' object containing a creation
date, the tagger name and e-mail, a tag message and an optional GnuPG
signature.  Unless
`-m ` or `-F ` is given, an editor is started for the user to
type in the tag message.

Otherwise just a tag reference for the SHA-1 object name of the commit
object is created (i.e. a lightweight tag).

Unless `-f` is given, the named tag must not yet exist.

If `-m ` or `-F ` is given and `-a`, `-s`, and `-u `
are absent, `-a` is implied.

A GnuPG signed tag object will be created when `-s` or `-u
` is used.  When `-u ` is not used, the
committer identity for the current user is used to find the
GnuPG key for signing.  The configuration variable `gpg.program`
is used to specify custom GnuPG binary.

Tag objects (created with `-a`, `s`, or `-u`) are called "annotated"
tags; whereas a "lightweight" tag is simply a name for an object
(usually a commit object).

Annotated tags are meant for release while lightweight tags are meant
for private or temporary object labels. For this reason, some git
commands for naming objects (like `git describe`) will ignore
lightweight tags by default.


I suppose there's no interest in this anymore

thanks anyway,
Regards,
Daniele Segato

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

Re: Repo with only one file

[Johannes Sixt]

Am 8/7/2013 8:24, schrieb shawn wilson:> ... create a repo for one of
> these scripts and I'd like to keep the commit history.
>
> Ok, so:
> % find -type f ! -iname "webban.pl" | while read f; do git
> filter-branch -f --index-filter "git rm --cached --ignore-unmatch $f"
> HEAD ; done
>
> Which basically did it. But, I've got this one commit that seems to be
> orphaned - it doesn't change any files.

Try this:

  git filter-branch HEAD -- webban.pl

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

Re: [RFC PATCH] During a shallow fetch, prevent sending over unneeded objects

[Matthijs Kooijman]

Hi Junio,

I haven't got a reply to my mail yet. Could you have a look, so I can
update and resubmit my patch?

On Fri, Jul 12, 2013 at 09:11:57AM +0200, Matthijs Kooijman wrote:
> > [administrivia: you seem to have mail-followup-to that points at you
> > and the list; is that really needed???]
> > In your discussion (including the comment), you talk about "shallow
> > root" (I think that is the same as what we call "shallow boundary"),
> I think so, yes. I mean to refer to the commits referenced in
> .git/shallow, that have their parents "hidden".
Could you confirm that I got the terms right here (or is the shallow
boundary the first hidden commit?)

> > but in this added block, there is nothing that checks CLIENT_SHALLOW
> > or SHALLOW flags to special case that.
> >
> > Is it a good idea to unconditionally do this for all "have"
> > revisions?
> That's what I meant in my mail with "applying the fix unconditionally" -
> there is probably some check needed (I discussed a few options in the
> mail as well).
>
> Note that this entire do_rev_list function is only called when there are
> shallow revisions involved, so there is also a basic "only when shallow"
> check in place.

My proposal was to only apply the fix for all have revisions when the
previous history traversal came across some shallow boundary commits. If
this happens, then that shallow boundary commit will be a "new" one and
it will have prevented the history traversal from finding the full list
of relevant "have" commits. In this case, we should just use all "have"
commits instead.

Now, looking at the code, I see a few options for detecting this case:

 1 Modify mark_edges_uninteresting to return a boolean (or have an
   output argument) if any of the commits in the list of commits to find
   (not the edges) is a shallow boundary.
 2 Modify mark_edges_uninteresting to have a "show_shallow" argument
   that gets called for every shallow boundary. The show_shallow
   function passed would then simply keep a boolean if it is passed at
   least once.
 3 Add another loop over the commits _after_ the call to
   mark_edges_uninteresting, that simply looks for any shallow boundary
   commit.

The last option seems sensible to me, since it prevents modifying the
somewhat generic mark_edges_uninteresting function for this specific
usecase. On the other hand, it does mean that the list of commits is
looped twice, not sure what that means for performance.

Before I go and implement one of these, which option seems best to you?

Gr.

Matthijs
--
To unsubscribe from this list: send the line "unsubscribe 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] die_with_status: use "printf '%s\n'", not "echo"

[Thomas Rast]

Matthieu Moy  writes:

> Thomas Rast  writes:
>
>>> +   grep -v "   " error
>>
>> Umm, doesn't that only test that _some_ line in the error does not
>> contain a tab?
>
> Indeed. It does work as the error message is just a one-liner, but let's
> be robust anyway.

Err, right.  I actually applied the patch and verified that the test
"erroneously" passed without the fix, but that's simply because I use
bash instead of dash.  The error message indeed is only one line, the
rest of the rebase output goes to stdout.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] die_with_status: use "printf '%s\n'", not "echo"

[Matthieu Moy]

Some implementations of 'echo' (e.g. dash's built-in) interpret
backslash sequences in their arguments.

This triggered at least one bug: the error message of "rebase -i" was
turning \t in commit messages into actual tabulations. There may be
others.

Using "printf '%s\n'" instead avoids this bad behavior, and is the form
used by the "say" function.

Noticed-by: David Kastrup 
Signed-off-by: Matthieu Moy 
Signed-off-by: Junio C Hamano 
---
Changed the "grep" function to be more accurate. The test still
catches the old failure and pass after the fix.

 git-sh-setup.sh   |  2 +-
 t/t3404-rebase-interactive.sh | 13 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 7a964ad..e15be51 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -53,7 +53,7 @@ die () {
 die_with_status () {
status=$1
shift
-   echo >&2 "$*"
+   printf >&2 '%s\n' "$*"
exit "$status"
 }
 
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 49ccb38..4dbeddb 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -976,4 +976,17 @@ test_expect_success 'rebase -i with --strategy and -X' '
test $(cat file1) = Z
 '
 
+test_expect_success 'rebase -i error on commits with \ in message' '
+   current_head=$(git rev-parse HEAD)
+   test_when_finished "git rebase --abort; git reset --hard $current_head; 
rm -f error" &&
+   test_commit TO-REMOVE will-conflict old-content &&
+   test_commit "\temp" will-conflict new-content dummy &&
+   (
+   EDITOR=true &&
+   export EDITOR &&
+   test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
+   ) &&
+   test_expect_code 1 grep  "  emp" error
+'
+
 test_done
-- 
1.8.3.3.797.gb72c616

--
To unsubscribe from this list: send the line "unsubscribe 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] die_with_status: use "printf '%s\n'", not "echo"

[Matthieu Moy]

Thomas Rast  writes:

>> +grep -v "   " error
>
> Umm, doesn't that only test that _some_ line in the error does not
> contain a tab?

Indeed. It does work as the error message is just a one-liner, but let's
be robust anyway.

> Whereas you need to test that _no_ line contains emp, or some
> such.  Perhaps as
>
>   ! grep -v " emp" error

Err, then the -v should be remove. Also, I'll use test_expect_code 1
instead of ! to catch other grep failures, just in case.

Thanks, new patch comming.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] l10n: de.po: translate 5 messages

[Jeff King]

On Wed, Aug 07, 2013 at 11:17:08AM +0200, Thomas Rast wrote:

> This results from Peff's c17592a (commit: tweak empty cherry pick advice
> for sequencer, 2013-07-26):
> 
>   diff --git a/builtin/commit.c b/builtin/commit.c
>   index a17a5df..39717d5 100644
>   --- a/builtin/commit.c
>   +++ b/builtin/commit.c
>   @@ -62,8 +62,18 @@
>"If you wish to commit it anyway, use:\n"
>"\n"
>"git commit --allow-empty\n"
>   +"\n");
>   +
>   +static const char empty_cherry_pick_advice_single[] =
>   +N_("Otherwise, please use 'git reset'\n");
>   +
>   +static const char empty_cherry_pick_advice_multi[] =
>   +N_("If you wish to skip this commit, use:\n"
>"\n"
>   -"Otherwise, please use 'git reset'\n");
>   +"git reset\n"
>   +"\n"
>   +"Then \"git cherry-pick --continue\" will resume cherry-picking\n"
>   +"the remaining commits.\n");
>
>static const char *use_message_buffer;
>static const char commit_editmsg[] = "COMMIT_EDITMSG";
>
> [...]
>
> So it seems that concatenating sentences indeed falls into a gray area
> between "avoid sentence lego" and "split at paragraphs".  And Peff's
> style of splitting it saves the translators work because the first part
> of the message is shared among two code paths.

Yeah, I was mainly trying to avoid repeating myself, since the first
part of the message would be the same (and I did not want them to fall
out of sync).

However, I do not mind changing it if translators would prefer to see
the whole message as a single string. I agree it's a gray area.

-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] l10n: de.po: translate 5 messages

[Thomas Rast]

Ralf Thielow  writes:
> 

The changes look good to me.  The following is purely about the original
English messages.

>  #: builtin/commit.c:62
> -#, fuzzy
>  msgid ""
>  "The previous cherry-pick is now empty, possibly due to conflict 
> resolution.\n"
>  "If you wish to commit it anyway, use:\n"
>  "\n"
>  "git commit --allow-empty\n"
> @@ -4014,25 +4012,30 @@ msgstr ""
>  "Konfliktauflösung.\n"
>  "Wenn Sie dies trotzdem committen wollen, benutzen Sie:\n"
>  "\n"
>  "git commit --allow-empty\n"
>  "\n"
> -"Andernfalls benutzen Sie bitte 'git reset'\n"
>  
>  #: builtin/commit.c:69
>  msgid "Otherwise, please use 'git reset'\n"
> -msgstr ""
> +msgstr "Andernfalls benutzen Sie bitte 'git reset'\n"

This results from Peff's c17592a (commit: tweak empty cherry pick advice
for sequencer, 2013-07-26):

  diff --git a/builtin/commit.c b/builtin/commit.c
  index a17a5df..39717d5 100644
  --- a/builtin/commit.c
  +++ b/builtin/commit.c
  @@ -62,8 +62,18 @@
   "If you wish to commit it anyway, use:\n"
   "\n"
   "git commit --allow-empty\n"
  +"\n");
  +
  +static const char empty_cherry_pick_advice_single[] =
  +N_("Otherwise, please use 'git reset'\n");
  +
  +static const char empty_cherry_pick_advice_multi[] =
  +N_("If you wish to skip this commit, use:\n"
   "\n"
  -"Otherwise, please use 'git reset'\n");
  +"git reset\n"
  +"\n"
  +"Then \"git cherry-pick --continue\" will resume cherry-picking\n"
  +"the remaining commits.\n");
   
   static const char *use_message_buffer;
   static const char commit_editmsg[] = "COMMIT_EDITMSG";


I was wondering for a while if this was a smart move, based on the usual
observation that it is better to translate things in one chunk because
many languages have more interdependencies than English does.  The
gettext manual says:

 puts ("Apollo 13 scenario: Stack overflow handling failed.");
 puts ("On the next stack overflow we will crash!!!");

  Should these two statements merged into a single one? I would recommend
  to merge them if the two sentences are related to each other, because
  then it makes it easier for the translator to understand and translate
  both.  On the other hand, if one of the two messages is a stereotypic
  one, occurring in other places as well, you will do a favour to the
  translator by not merging the two.
[...]
 Translatable strings should be limited to one paragraph; don't let a
  single message be longer than ten lines.

So it seems that concatenating sentences indeed falls into a gray area
between "avoid sentence lego" and "split at paragraphs".  And Peff's
style of splitting it saves the translators work because the first part
of the message is shared among two code paths.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe 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] die_with_status: use "printf '%s\n'", not "echo"

[Thomas Rast]

Matthieu Moy  writes:

> At least GNU echo interprets backslashes in its arguments.
>
> This triggered at least one bug: the error message of "rebase -i" was
> turning \t in commit messages into actual tabulations. There may be
> others.
>
> Using "printf '%s\n'" instead avoids this bad behavior, and is the form
> used by the "say" function.
>
> Noticed-by: David Kastrup 
> Signed-off-by: Matthieu Moy 
[...]
> +test_expect_success 'rebase -i error on commits with \ in message' '
> + current_head=$(git rev-parse HEAD)
> + test_when_finished "git rebase --abort; git reset --hard $current_head; 
> rm -f error" &&
> + test_commit TO-REMOVE will-conflict old-content &&
> + test_commit "\temp" will-conflict new-content dummy &&
> + (
> + EDITOR=true &&
> + export EDITOR &&
> + test_must_fail git rebase -i HEAD^ --onto HEAD^^ 2>error
> + ) &&
> + grep -v "   " error

Umm, doesn't that only test that _some_ line in the error does not
contain a tab?

Whereas you need to test that _no_ line contains emp, or some
such.  Perhaps as

  ! grep -v "   emp" error

> +'
> +
>  test_done

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe 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] replace: forbid replacing an object with one of a different type

[Thomas Rast]

Christian Couder  writes:

> Users replacing an object with one of a different type were not
> prevented to do so, even if it was obvious, and stated in the doc,
> that bad things would result from doing that.
>
> To avoid mistakes, it is better to just forbid that though.
>
> The doc will be updated in a later patch.
>
> Signed-off-by: Christian Couder 

Feel free to steal some of my other email for the commit message, to
write down for posterity that reverting would not really be a useful
step.

The patch looks good to me.

> If this patch is considered useful, I will update the doc and
> maybe add tests.
>
>  builtin/replace.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 59d3115..0246ab3 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -85,6 +85,7 @@ static int replace_object(const char *object_ref, const 
> char *replace_ref,
> int force)
>  {
>   unsigned char object[20], prev[20], repl[20];
> + enum object_type obj_type, repl_type;
>   char ref[PATH_MAX];
>   struct ref_lock *lock;
>  
> @@ -100,6 +101,14 @@ static int replace_object(const char *object_ref, const 
> char *replace_ref,
>   if (check_refname_format(ref, 0))
>   die("'%s' is not a valid ref name.", ref);
>  
> + obj_type = sha1_object_info(object, NULL);
> + repl_type = sha1_object_info(repl, NULL);
> + if (obj_type != repl_type)
> + die("Object ref '%s' is of type '%s'\n"
> + "while replace ref '%s' is of type '%s'.",
> + object_ref, typename(obj_type),
> + replace_ref, typename(repl_type));
> +
>   if (read_ref(ref, prev))
>   hashclr(prev);
>   else if (!force)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe 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 12/19] read-cache: read index-v5

[Thomas Gummerer]

Duy Nguyen  writes:

> A little bit more..
>
> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer  
> wrote:
>> +static void ce_queue_push(struct cache_entry **head,
>> +struct cache_entry **tail,
>> +struct cache_entry *ce)
>> +{
>> +   if (!*head) {
>> +   *head = *tail = ce;
>> +   (*tail)->next = NULL;
>> +   return;
>> +   }
>> +
>> +   (*tail)->next = ce;
>> +   ce->next = NULL;
>> +   *tail = (*tail)->next;
>
> No no no. "next" is for name-hash.c don't "reuse" it here. And I don't
> think you really need to maintain a linked list of cache_entries by
> the way. More on read_entries comments below..

You're right, I don't need it for the reading code.  I do need to keep a
list of cache-entries for writing though, where a linked list is best
suited for the task.  I'll use a next_ce pointer for that.

>> +}
>> +
>> +static struct cache_entry *ce_queue_pop(struct cache_entry **head)
>> +{
>> +   struct cache_entry *ce;
>> +
>> +   ce = *head;
>> +   *head = (*head)->next;
>> +   return ce;
>> +}
>
> Same as ce_queue_push.
>
>> +static int read_entries(struct index_state *istate, struct directory_entry 
>> **de,
>> +   unsigned int *entry_offset, void **mmap,
>> +   unsigned long mmap_size, unsigned int *nr,
>> +   unsigned int *foffsetblock)
>> +{
>> +   struct cache_entry *head = NULL, *tail = NULL;
>> +   struct conflict_entry *conflict_queue;
>> +   struct cache_entry *ce;
>> +   int i;
>> +
>> +   conflict_queue = NULL;
>> +   if (read_conflicts(&conflict_queue, *de, mmap, mmap_size) < 0)
>> +   return -1;
>> +   for (i = 0; i < (*de)->de_nfiles; i++) {
>> +   if (read_entry(&ce,
>> +  *de,
>> +  entry_offset,
>> +  mmap,
>> +  mmap_size,
>> +  foffsetblock) < 0)
>> +   return -1;
>> +   ce_queue_push(&head, &tail, ce);
>> +   *foffsetblock += 4;
>> +
>> +   /*
>> +* Add the conflicted entries at the end of the index file
>> +* to the in memory format
>> +*/
>> +   if (conflict_queue &&
>> +   (conflict_queue->entries->flags & CONFLICT_CONFLICTED) 
>> != 0 &&
>> +   !cache_name_compare(conflict_queue->name, 
>> conflict_queue->namelen,
>> +   ce->name, ce_namelen(ce))) {
>> +   struct conflict_part *cp;
>> +   cp = conflict_queue->entries;
>> +   cp = cp->next;
>> +   while (cp) {
>> +   ce = convert_conflict_part(cp,
>> +   conflict_queue->name,
>> +   conflict_queue->namelen);
>> +   ce_queue_push(&head, &tail, ce);
>> +   conflict_part_head_remove(&cp);
>> +   }
>> +   conflict_entry_head_remove(&conflict_queue);
>> +   }
>
> I start to wonder if separating staged entries is a good idea. It
> seems to make the code more complicated. The good point about conflict
> section at the end of the file is you can just truncate() it out.
> Another way is putting staged entries in fileentries, sorted
> alphabetically then by stage number, and a flag indicating if the
> entry is valid. When you remove resolve an entry, just set the flag to
> invalid (partial write), so that read code will skip it.
>
> I think this approach is reasonably cheap (unless there are a lot of
> conflicts) and it simplifies this piece of code. truncate() may be
> overrated anyway. In my experience, I "git add " as soon as I
> resolve  (so that "git diff" shrinks). One entry at a time, one
> index write at a time. I don't think I ever resolve everything then
> "git add -u .", which is where truncate() shines because staged
> entries are removed all at once. We should optimize for one file
> resolution at a time, imo.
>
>> +   }
>> +
>> +   *de = (*de)->next;
>> +
>> +   while (head) {
>> +   if (*de != NULL
>> +   && strcmp(head->name, (*de)->pathname) > 0) {
>> +   read_entries(istate,
>> +de,
>> +entry_offset,
>> +mmap,
>> +mmap_size,
>> +nr,
>> +foffsetblock);
>> +   } else {
>> +   ce = ce_queue_pop(&head);
>> +   set_index_entry(istate, *nr, ce);
>> +  

Re: [PATCH v2 12/19] read-cache: read index-v5

[Thomas Gummerer]

Duy Nguyen  writes:

> On Sat, Jul 13, 2013 at 12:26 AM, Thomas Gummerer  
> wrote:
>> +struct directory_entry {
>> +   struct directory_entry *next;
>> +   struct directory_entry *next_hash;
>> +   struct cache_entry *ce;
>> +   struct cache_entry *ce_last;
>> +   struct conflict_entry *conflict;
>> +   struct conflict_entry *conflict_last;
>> +   unsigned int conflict_size;
>> +   unsigned int de_foffset;
>> +   unsigned int de_cr;
>> +   unsigned int de_ncr;
>> +   unsigned int de_nsubtrees;
>> +   unsigned int de_nfiles;
>> +   unsigned int de_nentries;
>> +   unsigned char sha1[20];
>> +   unsigned short de_flags;
>> +   unsigned int de_pathlen;
>> +   char pathname[FLEX_ARRAY];
>> +};
>> +
>> +struct conflict_part {
>> +   struct conflict_part *next;
>> +   unsigned short flags;
>> +   unsigned short entry_mode;
>> +   unsigned char sha1[20];
>> +};
>> +
>> +struct conflict_entry {
>> +   struct conflict_entry *next;
>> +   unsigned int nfileconflicts;
>> +   struct conflict_part *entries;
>> +   unsigned int namelen;
>> +   unsigned int pathlen;
>> +   char name[FLEX_ARRAY];
>> +};
>> +
>> +struct ondisk_conflict_part {
>> +   unsigned short flags;
>> +   unsigned short entry_mode;
>> +   unsigned char sha1[20];
>> +};
>
> These new structs should probably be in read-cache-v5.c, or read-cache.h

Makes sense, thanks.

>>  #define cache_entry_size(len) (offsetof(struct cache_entry,name) + (len) + 
>> 1)
>> +#define directory_entry_size(len) (offsetof(struct 
>> directory_entry,pathname) + (len) + 1)
>> +#define conflict_entry_size(len) (offsetof(struct conflict_entry,name) + 
>> (len) + 1)
>
> These are used by read-cache-v5.c only so far. I'd say move them to
> read-cache.h or read-cache-v5.c together with the new structs.

Thanks.

>> +struct ondisk_cache_entry {
>> +   unsigned short flags;
>> +   unsigned short mode;
>> +   struct cache_time mtime;
>> +   unsigned int size;
>> +   int stat_crc;
>> +   unsigned char sha1[20];
>> +};
>> +
>> +struct ondisk_directory_entry {
>> +   unsigned int foffset;
>> +   unsigned int cr;
>> +   unsigned int ncr;
>> +   unsigned int nsubtrees;
>> +   unsigned int nfiles;
>> +   unsigned int nentries;
>> +   unsigned char sha1[20];
>> +   unsigned short flags;
>> +};
>
> Perhaps use uint32_t, uint16_t and friends for all on-disk structures?

We got this in the makefile, so I think we should be fine without it.
It still makes sense for clarity though I think.

 ifdef NO_UINTMAX_T
   BASIC_CFLAGS += -Duintmax_t=uint32_t
 endif

While at it I'll make the code for v[234] use them too.

>> +static struct directory_entry *read_directories(unsigned int *dir_offset,
>> +   unsigned int *dir_table_offset,
>> +   void *mmap,
>> +   int mmap_size)
>> +{
>> +   int i, ondisk_directory_size;
>> +   uint32_t *filecrc, *beginning, *end;
>> +   struct directory_entry *current = NULL;
>> +   struct ondisk_directory_entry *disk_de;
>> +   struct directory_entry *de;
>> +   unsigned int data_len, len;
>> +   char *name;
>> +
>> +   /*
>> +* Length of pathname + nul byte for termination + size of
>> +* members of ondisk_directory_entry. (Just using the size
>> +* of the struct doesn't work, because there may be padding
>> +* bytes for the struct)
>> +*/
>> +   ondisk_directory_size = sizeof(disk_de->flags)
>> +   + sizeof(disk_de->foffset)
>> +   + sizeof(disk_de->cr)
>> +   + sizeof(disk_de->ncr)
>> +   + sizeof(disk_de->nsubtrees)
>> +   + sizeof(disk_de->nfiles)
>> +   + sizeof(disk_de->nentries)
>> +   + sizeof(disk_de->sha1);
>> +   name = ptr_add(mmap, *dir_offset);
>> +   beginning = ptr_add(mmap, *dir_table_offset);
>> +   end = ptr_add(mmap, *dir_table_offset + 4);
>> +   len = ntoh_l(*end) - ntoh_l(*beginning) - ondisk_directory_size - 5;
>> +   disk_de = ptr_add(mmap, *dir_offset + len + 1);
>> +   de = directory_entry_from_ondisk(disk_de, name, len);
>> +   de->next = NULL;
>> +
>> +   data_len = len + 1 + ondisk_directory_size;
>> +   filecrc = ptr_add(mmap, *dir_offset + data_len);
>> +   if (!check_crc32(0, ptr_add(mmap, *dir_offset), data_len, 
>> ntoh_l(*filecrc)))
>> +   goto unmap;
>> +
>> +   *dir_table_offset += 4;
>> +   *dir_offset += data_len + 4; /* crc code */
>> +
>> +   current = de;
>> +   for (i = 0; i < de->de_nsubtrees; i++) {
>> +   current->next = read_directories(dir_offset, 
>> dir_table_offset,
>> +   mmap, mmap_size);
>> +   while (current->next)
>> +  

Re: [RFH/PATCH 4/4] OPT__FORCE(): clarify its expected use by using OPT_COUNTUP

[Stefan Beller]

On 08/07/2013 01:31 AM, Junio C Hamano wrote:
> The parseopt parsing for OPT__FORCE() is implemented in terms of
> OPT_BOOLEAN() and users of it can take advantage of the "counting
> up" behaviour to implement increasing levels of forcefulness by
> differentiating "git cmd -f" and "git cmd -f -f".
> 
> Clarify this by explicitly using OPT_COUNTUP() instead.
> 
> Signed-off-by: Junio C Hamano 
> ---
> 
>  * This _should_ be done with a similar audit of existing callers,
>but I ran out of concentration.
> 
>  parse-options.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/parse-options.h b/parse-options.h
> index 78f52c2..1eeb0d9 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -238,7 +238,7 @@ extern int parse_opt_noop_cb(const struct option *, const 
> char *, int);
>   { OPTION_CALLBACK, 'q', "quiet", (var), NULL, N_("be more quiet"), \
> PARSE_OPT_NOARG, &parse_opt_verbosity_cb, 0 }
>  #define OPT__DRY_RUN(var, h)  OPT_BOOL('n', "dry-run", (var), (h))
> -#define OPT__FORCE(var, h)OPT_BOOLEAN('f', "force",   (var), (h))
> +#define OPT__FORCE(var, h)OPT_COUNTUP('f', "force",   (var), (h))
>  #define OPT__ABBREV(var)  \
>   { OPTION_CALLBACK, 0, "abbrev", (var), N_("n"), \
> N_("use  digits to display SHA-1s"),   \
> 

We need the COUNTUP, because in builtin/clean.c we have
OPT__FORCE(&force, N_("force")),
...
if (force > 1)
rm_flags = 0;

So a OPT_BOOL definitely doesn't cut it.
Now that I started reviewing the OPT_FORCE parts, I realize
there is still an error in the patch, which needed correction.
(branch, commit, name-rev: ease up boolean conditions):

-   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
!!unset_upstream > 1)
+   if (force_create + list + unset_upstream +
+   !!delete + !!rename + !!new_upstream > 1)
usage_with_options(builtin_branch_usage, options);

force_create is set via OPT_FORCE as well, so we cannot remove the !! before 
the force_create,
hence we'd only remove it from list and unset_upstream, which are set by 
OPT_BOOL.

-- 8< --
From: Stefan Beller 
Date: Wed, 7 Aug 2013 09:32:25 +0200
Subject: [PATCH] branch, commit, name-rev: ease up boolean conditions

Now that the variables are set by OPT_BOOL, which makes sure
to have the values being 0 or 1 after parsing, we do not need
the double negation to map any other value to 1 for integer
variables.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/branch.c   | 3 ++-
 builtin/commit.c   | 2 +-
 builtin/name-rev.c | 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 4daed0b..0903763 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -872,7 +872,8 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
if (with_commit || merge_filter != NO_FILTER)
list = 1;
 
-   if (!!delete + !!rename + !!force_create + !!list + !!new_upstream + 
!!unset_upstream > 1)
+   if (!!delete + !!rename + !!force_create + !!new_upstream +
+   list + unset_upstream > 1)
usage_with_options(builtin_branch_usage, options);
 
if (abbrev == -1)
diff --git a/builtin/commit.c b/builtin/commit.c
index c20426b..b0f86c8 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1072,7 +1072,7 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
if (patch_interactive)
interactive = 1;
 
-   if (!!also + !!only + !!all + !!interactive > 1)
+   if (also + only + all + interactive > 1)
die(_("Only one of --include/--only/--all/--interactive/--patch 
can be used."));
if (argc == 0 && (also || (only && !amend)))
die(_("No paths with --include/--only does not make sense."));
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index a908a34..20fcf8c 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -331,7 +331,7 @@ int cmd_name_rev(int argc, const char **argv, const char 
*prefix)
 
git_config(git_default_config, NULL);
argc = parse_options(argc, argv, prefix, opts, name_rev_usage, 0);
-   if (!!all + !!transform_stdin + !!argc > 1) {
+   if (all + transform_stdin + !!argc > 1) {
error("Specify either a list, or --all, not both!");
usage_with_options(name_rev_usage, opts);
}
-- 
1.8.4.rc0.16.g7fca822.dirty




signature.asc
Description: OpenPGP digital signature