* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 7:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <7vfxpq3559.fsf@gitster.siamese.dyndns.org>
On Thu, Jul 31, 2008 at 12:05:22AM -0700, Junio C Hamano wrote:
> We can probably pick up the result revision parser parsed out of
> revs.diffopt, and then tweak "opt" with it, perhaps like this.
That is a sensible solution for this option, but I have to wonder: how
many other such ineffective options are hiding? How many of them
actually have a matching meaning in git-blame? E.g., what does "git
blame --name-only" mean?
Perhaps we should simply not worry about those ones, as people are
unlikely to try using them, and it is easy to say "has no impact,
because it doesn't make sense with blame." The truly confusing ones are
ones you _expect_ to do something, but don't (like
--find-copies-harder).
I took a look at implementing a "don't parse the diff options" flag, but
it is much larger than that. The revision parser understands a lot of
options that don't really make sense for blame (or shortlog), like
"--full-diff". So perhaps it is best to just fix this one (which we have
actually had a bug report about) and not worry about the rest.
-Peff
^ permalink raw reply
* Re: [PATCH v3] Advertise the ability to abort a commit
From: Anders Melchiorsen @ 2008-07-31 7:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster
In-Reply-To: <20080731055024.GA17652@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I still prefer a shortened version of these three lines, as I
> mentioned earlier.
Yeah, and I obviously didn't :-). I think the line wrapped, run-on
sentence makes it look more busy, even if it is shorter. Here is a
final compromise proposal:
# Enter a commit message for your changes. Use an empty one to abort.
# (Comment lines starting with '#' will not be included)
As this is mostly a matter of personal opinion, I will stop here.
Cheers,
Anders.
^ permalink raw reply
* Re: [PATCH v3] Advertise the ability to abort a commit
From: Jeff King @ 2008-07-31 7:32 UTC (permalink / raw)
To: Anders Melchiorsen; +Cc: git, gitster
In-Reply-To: <87sktqfr6a.fsf@cup.kalibalik.dk>
On Thu, Jul 31, 2008 at 09:28:45AM +0200, Anders Melchiorsen wrote:
> > I still prefer a shortened version of these three lines, as I
> > mentioned earlier.
>
> Yeah, and I obviously didn't :-). I think the line wrapped, run-on
> sentence makes it look more busy, even if it is shorter. Here is a
> final compromise proposal:
OK, I wasn't sure if I was being disagreed with or overlooked. ;)
> # Enter a commit message for your changes. Use an empty one to abort.
> # (Comment lines starting with '#' will not be included)
I don't like that as well as mine, but we are well into the realm of
personal preference. I am fine with whatever the list (or Junio)
decides.
-Peff
^ permalink raw reply
* Re: [PATCH v3] Advertise the ability to abort a commit
From: Jeff King @ 2008-07-31 7:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Anders Melchiorsen, git
In-Reply-To: <7vwsj23896.fsf@gitster.siamese.dyndns.org>
On Wed, Jul 30, 2008 at 10:58:13PM -0700, Junio C Hamano wrote:
> >> "# Please enter the commit message for your changes.\n"
> >> + "# To abort the commit, use an empty commit message.\n"
> >> "# (Comment lines starting with '#' will ");
> >
> > I still prefer a shortened version of these three lines, as I mentioned
> > earlier.
>
> I tend to agree; please make it so ;-)
Hmm, I didn't realize you had already applied the original patch. Here
is my previous patch, rebased on top of the current master.
I like this wording, but there is perhaps some disagreement. I will let
you apply, tweak, or ignore as you desire. :)
Note that this still has the error message change that Anders put in a
later patch, but is not in master. Should that be a separate patch (I
really didn't anticipate this much discussion for such a simple change,
but I think there is a rule of thumb about patch size and bike
sheds...)?
-- >8 --
Compact commit template message
We recently let the user know explicitly that an empty
commit message will abort the commit. However, this adds yet
another line to the template; let's rephrase and re-wrap so
that this fits back on two lines.
This patch also makes the "fatal: empty commit message?"
warning a bit less scary, since this is now a "feature"
instead of an error. However, we retain the non-zero exit
status to indicate to callers that nothing was committed.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-commit.c | 19 ++++++++++++-------
t/t7502-commit.sh | 11 +++++------
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/builtin-commit.c b/builtin-commit.c
index f7c053a..b783e6e 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
fprintf(fp,
"\n"
- "# Please enter the commit message for your changes.\n"
- "# To abort the commit, use an empty commit message.\n"
- "# (Comment lines starting with '#' will ");
+ "# Please enter the commit message for your changes.");
if (cleanup_mode == CLEANUP_ALL)
- fprintf(fp, "not be included)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be ignored, and an empty"
+ " message aborts the commit.\n");
else /* CLEANUP_SPACE, that is. */
- fprintf(fp, "be kept.\n"
- "# You can remove them yourself if you want to)\n");
+ fprintf(fp,
+ " Lines starting\n"
+ "# with '#' will be kept; you may remove them"
+ " yourself if you want to.\n"
+ "# An empty message aborts the commit.\n");
if (only_include_assumed)
fprintf(fp, "# %s\n", only_include_assumed);
@@ -1004,7 +1008,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
stripspace(&sb, cleanup_mode == CLEANUP_ALL);
if (sb.len < header_len || message_is_empty(&sb, header_len)) {
rollback_index_files();
- die("no commit message? aborting commit.");
+ fprintf(stderr, "Aborting commit due to empty commit message.\n");
+ exit(1);
}
strbuf_addch(&sb, '\0');
if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index f111263..3eb9fae 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -141,16 +141,15 @@ test_expect_success 'cleanup commit messages (strip,-F)' '
echo "sample
-# Please enter the commit message for your changes.
-# To abort the commit, use an empty commit message.
-# (Comment lines starting with '#' will not be included)" >expect
+# Please enter the commit message for your changes. Lines starting
+# with '#' will be ignored, and an empty message aborts the commit." >expect
test_expect_success 'cleanup commit messages (strip,-F,-e)' '
echo >>negative &&
{ echo;echo sample;echo; } >text &&
git commit -e -F text -a &&
- head -n 5 .git/COMMIT_EDITMSG >actual &&
+ head -n 4 .git/COMMIT_EDITMSG >actual &&
test_cmp expect actual
'
@@ -163,7 +162,7 @@ test_expect_success 'author different from committer' '
echo >>negative &&
git commit -e -m "sample"
- head -n 8 .git/COMMIT_EDITMSG >actual &&
+ head -n 7 .git/COMMIT_EDITMSG >actual &&
test_cmp expect actual
'
@@ -182,7 +181,7 @@ test_expect_success 'committer is automatic' '
# must fail because there is no change
test_must_fail git commit -e -m "sample"
) &&
- head -n 9 .git/COMMIT_EDITMSG | \
+ head -n 8 .git/COMMIT_EDITMSG | \
sed "s/^# Committer: .*/# Committer:/" >actual &&
test_cmp expect actual
'
--
1.6.0.rc1.169.g34ee
^ permalink raw reply related
* Re: git blame not respecting --find-copies-harder ?
From: Junio C Hamano @ 2008-07-31 7:36 UTC (permalink / raw)
To: Jeff King
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <20080731072149.GA2304@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I took a look at implementing a "don't parse the diff options" flag, but
> it is much larger than that. The revision parser understands a lot of
> options that don't really make sense for blame (or shortlog), like
> "--full-diff". So perhaps it is best to just fix this one (which we have
> actually had a bug report about) and not worry about the rest.
That reminds me of an issue with shortlog.
I often wish to do this:
git shortlog --since=3.day --all | sort | uniq -c
This is to catch a stupid mistake of (1) applying a few patches to
'master', (2) forking a new topic from 'master' and applying a few patches
there, (3) realizing a few commits on 'master' that haven't been pushed
out was faulty and rewrite the 'master' history. Such a new topic made in
step (2) must be rebased on the updated 'master' built in step (3);
otherwise merging the topic to 'next' will contaminate it with the old
version of patches that have been rewritten on 'master'.
Alas, shortlog does not take --all. Yes, I know
git log --since=3.day --all | git shortlog | sort | uniq -c
is an obvious workaround, but it is mildly irritating.
^ permalink raw reply
* [PATCH v2] revision traversal: show full history with merge simplification
From: Junio C Hamano @ 2008-07-31 8:17 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Roman Zippel, Martin Langhoff, Tim Harper, git
In-Reply-To: <7vej5b3ozz.fsf@gitster.siamese.dyndns.org>
The --full-history traversal keeps all merges in addition to non-merge
commits that touch paths in the given pathspec. This is useful to view
both sides of a merge in a topology like this:
A---M---o
/ /
---O---B
even when A and B makes identical change to the given paths. The revision
traversal without --full-history aims to come up with the simplest history
to explain the final state of the tree, and one of the side branches can
be pruned away.
The behaviour to keep all merges however is inconvenient if neither A nor
B touches the paths we are interested in. --full-history reduces the
topology to:
---O---M---o
in such a case, without removing M.
This adds a post processing phase on top of --full-history traversal to
remove needless merges from the resulting history.
The idea is to compute, for each commit in the "full history" result set,
the commit that should replace it in the simplified history. The commit
to replace it in the final history is determined as follows:
* In any case, we first figure out the replacement commits of parents of
the commit we are looking at. The commit we are looking at is
rewritten as if the replacement commits of its original parents are its
parents. While doing so, we reduce the redundant parents from the
rewritten parent list by not just removing the identical ones, but also
removing a parent that is an ancestor of another parent.
* After the above parent simplification, if the commit is a root commit,
an UNINTERESTING commit, a merge commit, or modifies the paths we are
interested in, then the replacement commit of the commit is itself. In
other words, such a commit is not dropped from the final result.
The first point above essentially means that the history is rewritten in
the bottom up direction. We can rewrite the parent list of a commit only
after we know how all of its parents are rewritten. This means that the
processing needs to happen on the full history (i.e. after limit_list()).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* Changes from the "quick hack" are:
- When the history is bounded at the bottom, the v1 patch did not
terminate, because it wanted to know the replacement for
UNINTERESTING parents of commits on revs->commit list, but these
parents were never processed. Oops.
- The option implies rewrite_parents. I was tempted to make it imply
"--parents" (which would make it always emit parent information as
well), but didn't.
- Toposort is still implied but it is done at the end.
- The code is more heavily commented.
- I do not think "--post-simplify" is particulary a good name, but I
couldn't come up with a good one. To mark it clearly not ready for
'master', I changed the name to a meaningless word for now ;-)
* Timings (best of 5 runs)
$ git rev-list --parents --full-history --topo-order HEAD -- kernel/printk.c
3.75user 0.47system 0:04.22elapsed 100%CPU (0avgtext+0avgdata 0maxresident)k
$ git rev-list --parents --oyoyo HEAD -- kernel/printk.c
4.31user 0.06system 0:04.37elapsed 99%CPU (0avgtext+0avgdata 0maxresident)k
$ git rev-list --parents --full-history HEAD -- kernel/printk.c | head -n 200
0.16user 0.02system 0:00.18elapsed 103%CPU (0avgtext+0avgdata 0maxresident)k
revision.c | 171 +++++++++++++++++++++++++++++++++++++++++++++++++++++-------
revision.h | 1 +
2 files changed, 152 insertions(+), 20 deletions(-)
diff --git a/revision.c b/revision.c
index 3897fec..3b59e02 100644
--- a/revision.c
+++ b/revision.c
@@ -1045,6 +1045,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
} else if (!strcmp(arg, "--topo-order")) {
revs->lifo = 1;
revs->topo_order = 1;
+ } else if (!strcmp(arg, "--oyoyo")) {
+ revs->post_simplify = 1;
+ revs->rewrite_parents = 1;
+ revs->simplify_history = 0;
+ revs->limited = 1;
} else if (!strcmp(arg, "--date-order")) {
revs->lifo = 0;
revs->topo_order = 1;
@@ -1378,6 +1383,150 @@ static void add_child(struct rev_info *revs, struct commit *parent, struct commi
l->next = add_decoration(&revs->children, &parent->object, l);
}
+static int remove_duplicate_parents(struct commit *commit)
+{
+ struct commit_list **pp, *p;
+ int surviving_parents;
+
+ /* Examine existing parents while marking ones we have seen... */
+ pp = &commit->parents;
+ while ((p = *pp) != NULL) {
+ struct commit *parent = p->item;
+ if (parent->object.flags & TMP_MARK) {
+ *pp = p->next;
+ continue;
+ }
+ parent->object.flags |= TMP_MARK;
+ pp = &p->next;
+ }
+ /* count them while clearing the temporary mark */
+ surviving_parents = 0;
+ for (p = commit->parents; p; p = p->next) {
+ p->item->object.flags &= ~TMP_MARK;
+ surviving_parents++;
+ }
+ return surviving_parents;
+}
+
+static struct commit_list **post_simplify_one(struct commit *commit, struct commit_list **tail)
+{
+ struct commit_list *p;
+ int cnt;
+
+ /*
+ * We store which commit each one simplifies to in its util field.
+ * Have we handled this one?
+ */
+ if (commit->util)
+ return tail;
+
+ /*
+ * An UNINTERESTING commit simplifies to itself, so does a
+ * root commit. We do not rewrite parents of such commit
+ * anyway.
+ */
+ if ((commit->object.flags & UNINTERESTING) || !commit->parents) {
+ commit->util = commit;
+ return tail;
+ }
+
+ /*
+ * Do we know what commit all of our parents should be rewritten to?
+ * Otherwise we are not ready to rewrite this one yet.
+ */
+ for (cnt = 0, p = commit->parents; p; p = p->next) {
+ if (!p->item->util) {
+ tail = &commit_list_insert(p->item, tail)->next;
+ cnt++;
+ }
+ }
+ if (cnt)
+ return tail;
+
+ /*
+ * Rewrite our list of parents.
+ */
+ for (p = commit->parents; p; p = p->next)
+ p->item = p->item->util;
+ cnt = remove_duplicate_parents(commit);
+
+ /*
+ * It is possible that we are a merge and one side branch
+ * does not have any commit that touches the given paths;
+ * in such a case, the immediate parents will be rewritten
+ * to different commits.
+ *
+ * o----X X: the commit we are looking at;
+ * / / o: a commit that touches the paths;
+ * ---o----'
+ *
+ * Further reduce the parents by removing redundant parents.
+ */
+ if (1 < cnt) {
+ struct commit_list *h = reduce_heads(commit->parents);
+ cnt = commit_list_count(h);
+ free_commit_list(commit->parents);
+ commit->parents = h;
+ }
+
+ /*
+ * A commit simplifies to itself if it is a root, if it is
+ * UNINTERESTING, if it touches the given paths, or if it is a
+ * merge and its parents simplifies to more than one commits
+ * (the first two cases are already handled at the beginning of
+ * this function).
+ *
+ * Otherwise, it simplifies to what its sole parent simplifies to.
+ */
+ if (!cnt ||
+ (commit->object.flags & UNINTERESTING) ||
+ !(commit->object.flags & TREESAME) ||
+ (1 < cnt))
+ commit->util = commit;
+ else
+ commit->util = commit->parents->item->util;
+ return tail;
+}
+
+static void post_simplify(struct rev_info *revs)
+{
+ struct commit_list *list;
+ struct commit_list *yet_to_do, **tail;
+
+ /* feed the list reversed */
+ yet_to_do = NULL;
+ for (list = revs->commits; list; list = list->next)
+ commit_list_insert(list->item, &yet_to_do);
+ while (yet_to_do) {
+ list = yet_to_do;
+ yet_to_do = NULL;
+ tail = &yet_to_do;
+ while (list) {
+ struct commit *commit = list->item;
+ struct commit_list *next = list->next;
+ free(list);
+ list = next;
+ tail = post_simplify_one(commit, tail);
+ }
+ }
+
+ /* clean up the result, removing the simplified ones */
+ list = revs->commits;
+ revs->commits = NULL;
+ tail = &revs->commits;
+ while (list) {
+ struct commit *commit = list->item;
+ struct commit_list *next = list->next;
+ free(list);
+ list = next;
+ if (commit->util == commit)
+ tail = &commit_list_insert(commit, tail)->next;
+ }
+
+ /* sort topologically at the end */
+ sort_in_topological_order(&revs->commits, revs->lifo);
+}
+
static void set_children(struct rev_info *revs)
{
struct commit_list *l;
@@ -1418,6 +1567,8 @@ int prepare_revision_walk(struct rev_info *revs)
return -1;
if (revs->topo_order)
sort_in_topological_order(&revs->commits, revs->lifo);
+ if (revs->post_simplify)
+ post_simplify(revs);
if (revs->children.name)
set_children(revs);
return 0;
@@ -1450,26 +1601,6 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
}
}
-static void remove_duplicate_parents(struct commit *commit)
-{
- struct commit_list **pp, *p;
-
- /* Examine existing parents while marking ones we have seen... */
- pp = &commit->parents;
- while ((p = *pp) != NULL) {
- struct commit *parent = p->item;
- if (parent->object.flags & TMP_MARK) {
- *pp = p->next;
- continue;
- }
- parent->object.flags |= TMP_MARK;
- pp = &p->next;
- }
- /* ... and clear the temporary mark */
- for (p = commit->parents; p; p = p->next)
- p->item->object.flags &= ~TMP_MARK;
-}
-
static int rewrite_parents(struct rev_info *revs, struct commit *commit)
{
struct commit_list **pp = &commit->parents;
diff --git a/revision.h b/revision.h
index f64e8ce..953e69b 100644
--- a/revision.h
+++ b/revision.h
@@ -41,6 +41,7 @@ struct rev_info {
simplify_history:1,
lifo:1,
topo_order:1,
+ post_simplify:1,
tag_objects:1,
tree_objects:1,
blob_objects:1,
--
1.6.0.rc1.31.gf448e
^ permalink raw reply related
* Re: [PATCH v2] revision traversal: show full history with merge simplification
From: Junio C Hamano @ 2008-07-31 8:18 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Roman Zippel, Martin Langhoff, Tim Harper, git
In-Reply-To: <7vhca6zcuy.fsf@gitster.siamese.dyndns.org>
If you look at the output from the "kernel/printk.c" with this patch, you
would notice that there still are somewhat meaningless merges shown in the
history (e.g. scroll down to 185a257f2f73bcd89050ad02da5bedbc28fc43fa).
The mainline side keeps making steady changes to the path, but the side
branch that made tty_write_message available to others with b346671
([PATCH] Export tty_write_message() for GFS2 quota code, 2006-01-16) keeps
many "Merge from master" until it is merged back to the mainline, even
after the earlier change is reverted by 02630a1 ([GFS2] Remove dependance
on tty_write_message(), 2006-07-03).
I wonder if we can do something clever to reduce these pointless (from the
point of view of explaining kernel/printk.c's evolution, at least) merges
from the output. This might be another example of the reason why it is a
good thing that you keep teaching people: "On your xyzzy topic, you are
doing xyzzy development, not xyzzy development plus random changes ---
don't merge my tree into yours!", and we could dismiss these extra merges
we see in the output as artifacts from a bad practice, but as long as we
are spending extra cycles, it would be better if we could reduce such
clutter.
I am still undecided about the option name.
The existing --full-history is "show history fully without simplifying the
merge at all". This is "show history fully with merge simplification".
Perhaps --simplify-merges?
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 8:25 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <7v8wvi33ok.fsf@gitster.siamese.dyndns.org>
On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
> Alas, shortlog does not take --all. Yes, I know
>
> git log --since=3.day --all | git shortlog | sort | uniq -c
>
> is an obvious workaround, but it is mildly irritating.
Hmm. Could it be as simple as:
diff --git a/revision.c b/revision.c
index a843c42..eaa5572 100644
--- a/revision.c
+++ b/revision.c
@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
!strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
{
unkv[(*unkc)++] = arg;
- return 0;
+ return 1;
}
if (!prefixcmp(arg, "--max-count=")) {
That is, handle_revision_opt says "yes we parsed this, and it should be
gone" even though it still gets stuck in the "unknown" section to be
parsed by setup_revisions later.
-Peff
^ permalink raw reply related
* Re: git blame not respecting --find-copies-harder ?
From: Junio C Hamano @ 2008-07-31 8:35 UTC (permalink / raw)
To: Jeff King
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <20080731082553.GA19522@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
>
>> Alas, shortlog does not take --all. Yes, I know
>>
>> git log --since=3.day --all | git shortlog | sort | uniq -c
>>
>> is an obvious workaround, but it is mildly irritating.
>
> Hmm. Could it be as simple as:
>
> diff --git a/revision.c b/revision.c
> index a843c42..eaa5572 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
> {
> unkv[(*unkc)++] = arg;
> - return 0;
> + return 1;
> }
>
> if (!prefixcmp(arg, "--max-count=")) {
>
> That is, handle_revision_opt says "yes we parsed this, and it should be
> gone" even though it still gets stuck in the "unknown" section to be
> parsed by setup_revisions later.
Hmm, wouldn't that suggest it needs to return 1 when an option candidate
given to diff_opt_parse() turns out not to be a diff option and stuffed
back to unkv[] at the end of this function?
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Pierre Habouzit @ 2008-07-31 9:01 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <7v8wvizc16.fsf@gitster.siamese.dyndns.org>
[-- Attachment #1: Type: text/plain, Size: 3052 bytes --]
On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
> >
> >> Alas, shortlog does not take --all. Yes, I know
> >>
> >> git log --since=3.day --all | git shortlog | sort | uniq -c
> >>
> >> is an obvious workaround, but it is mildly irritating.
> >
> > Hmm. Could it be as simple as:
> >
> > diff --git a/revision.c b/revision.c
> > index a843c42..eaa5572 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
> > {
> > unkv[(*unkc)++] = arg;
> > - return 0;
> > + return 1;
> > }
> >
> > if (!prefixcmp(arg, "--max-count=")) {
> >
> > That is, handle_revision_opt says "yes we parsed this, and it should be
> > gone" even though it still gets stuck in the "unknown" section to be
> > parsed by setup_revisions later.
Ack this is a correct fix. I wonder how this even works with other
commands that use --all and stuff like that.
> Hmm, wouldn't that suggest it needs to return 1 when an option candidate
> given to diff_opt_parse() turns out not to be a diff option and stuffed
> back to unkv[] at the end of this function?
No, because diff-opts are our last chance, and those are _really_
unknown options. "--all" is different because revision machinery
acknowledge "yeah it's really for us, we recognize it ..." hence the 1
result "... but we have to parse it later so keep it in place".
There is no such thing with unknown option for the revision _and_ diff
machinery. They _must_ return 0 to say "no we really didn't know what
this is".
IOW, semantics is:
< 0 : error
0 : option was not directed at us, unknown (this allow to chain
option parsers.
> 0 : yeah this was for us, and we consumed this amount of arguments
in the process.
Here "--all" is clearly something that we _recognized_ hence fit in
the third case, even if we decide to still keep it in the "unk" array
(which is ill named, I kept its name, but it's not unknown options, it's
actually non option arguments that are to be deal with in the last
stage, setup_revisions here, or the command as a general rule).
Unknown options to both the revision _and_ diff option machineries are
clearly of the 2nd kind and must return 0. They put the option in 'unk'
as well (and here this is properly named) because this is how it
historically worked for many commands, and is here as a legacy
compatibility thing. parse-opt based parser (and git-shortlog has one)
does not uses that (and you can see it in parse_revision_opt that is
what such parser use: a result of `0' triggers a usage).
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 9:03 UTC (permalink / raw)
To: Junio C Hamano
Cc: sverre, Björn Steinbrink, Stephen R. van den Berg,
Git Mailinglist
In-Reply-To: <7v8wvizc16.fsf@gitster.siamese.dyndns.org>
On Thu, Jul 31, 2008 at 01:35:33AM -0700, Junio C Hamano wrote:
> > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
> > {
> > unkv[(*unkc)++] = arg;
> > - return 0;
> > + return 1;
> > }
> >
> > if (!prefixcmp(arg, "--max-count=")) {
> >
> > That is, handle_revision_opt says "yes we parsed this, and it should be
> > gone" even though it still gets stuck in the "unknown" section to be
> > parsed by setup_revisions later.
>
> Hmm, wouldn't that suggest it needs to return 1 when an option candidate
> given to diff_opt_parse() turns out not to be a diff option and stuffed
> back to unkv[] at the end of this function?
Not necessarily. The logic there goes:
1. it's not a revision option, so pass to diff
2. it's not a diff opt, so it is unknown; we parsed no options
3. the caller sees we failed to parse, so it barfs
but the logic here is:
1. it _is_ a revision option. Our handling of it is just a little odd,
in that we need to parse it later, when we are in setup_revisions.
So put it aside for now as a "revision" that just happens to look
like an option.
2. caller sees we parsed, and doesn't complain
3. caller later passes the "revision" to setup_revisions, which knows
what to do
Now my patch doesn't just operate on "--all", but rather several other
options including --no-walk. And maybe that is not right, and --all
should be handled separately (though I think --remotes would follow the
same logic). I'm not really sure why --no-walk is separated like that.
-Peff
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 9:05 UTC (permalink / raw)
To: Pierre Habouzit, Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg
In-Reply-To: <20080731090146.GA12867@artemis.madism.org>
On Thu, Jul 31, 2008 at 11:01:46AM +0200, Pierre Habouzit wrote:
> Ack this is a correct fix. I wonder how this even works with other
> commands that use --all and stuff like that.
--all is parsed separately in setup_revisions _before_ we call
handle_revision_opt. So the only codepath hitting that is parse_options
users who call parse_revision_opt (i.e., blame and shortlog).
-Peff
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Pierre Habouzit @ 2008-07-31 9:06 UTC (permalink / raw)
To: Junio C Hamano, Jeff King, sverre, Björn Steinbrink,
Stephen R. van den Berg
In-Reply-To: <20080731090146.GA12867@artemis.madism.org>
[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]
On Thu, Jul 31, 2008 at 09:01:46AM +0000, Pierre Habouzit wrote:
> On Thu, Jul 31, 2008 at 08:35:33AM +0000, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> >
> > > On Thu, Jul 31, 2008 at 12:36:59AM -0700, Junio C Hamano wrote:
> > >
> > >> Alas, shortlog does not take --all. Yes, I know
> > >>
> > >> git log --since=3.day --all | git shortlog | sort | uniq -c
> > >>
> > >> is an obvious workaround, but it is mildly irritating.
> > >
> > > Hmm. Could it be as simple as:
> > >
> > > diff --git a/revision.c b/revision.c
> > > index a843c42..eaa5572 100644
> > > --- a/revision.c
> > > +++ b/revision.c
> > > @@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> > > !strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
> > > {
> > > unkv[(*unkc)++] = arg;
> > > - return 0;
> > > + return 1;
> > > }
> > >
> > > if (!prefixcmp(arg, "--max-count=")) {
> > >
> > > That is, handle_revision_opt says "yes we parsed this, and it should be
> > > gone" even though it still gets stuck in the "unknown" section to be
> > > parsed by setup_revisions later.
>
> Ack this is a correct fix. I wonder how this even works with other
> commands that use --all and stuff like that.
Oh actually I know: the parse_revision_opt machinery (that is buggy
because of this 0 result) is used in git-blame where --all is
meaningless anyways, and ... git-shortlog only. The legacy way to parse
revision options does not treats `0' answers differently from `1'
actually, I wonder why, because this is probably wrong.
This was a regression, I suppose prior to its parse-optification
git-shortlog accepted --all (and I see no valid reason for it not to).
Thanks a lot to Jeff for the good catch.
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Pierre Habouzit @ 2008-07-31 9:15 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
In-Reply-To: <20080731090348.GB20691@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1976 bytes --]
On Thu, Jul 31, 2008 at 09:03:49AM +0000, Jeff King wrote:
> Not necessarily. The logic there goes:
>
> 1. it's not a revision option, so pass to diff
> 2. it's not a diff opt, so it is unknown; we parsed no options
> 3. the caller sees we failed to parse, so it barfs
>
> but the logic here is:
>
> 1. it _is_ a revision option. Our handling of it is just a little odd,
> in that we need to parse it later, when we are in setup_revisions.
> So put it aside for now as a "revision" that just happens to look
> like an option.
> 2. caller sees we parsed, and doesn't complain
> 3. caller later passes the "revision" to setup_revisions, which knows
> what to do
>
> Now my patch doesn't just operate on "--all", but rather several other
> options including --no-walk. And maybe that is not right, and --all
> should be handled separately (though I think --remotes would follow the
> same logic). I'm not really sure why --no-walk is separated like that.
--no-walk alters how add_pending_object_with_mode works, which is called
by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
--no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
or --remote are.
It's okay for parse_revision_opt to take out any option that doesn't
alter handle_revision_arg (as for parse_option based parsers it's the
sole thing setup_revision will be doing: consuming revisions in a loop
with handle_revision_arg), but only those.
The full list (that I made when I wrote parse_revision_opt) is actually:
pseudo arguments: --all --branches --tags --remotes --reflog
combining operator: --not
behavior modifying: --no-walk --do-walk
--
·O· Pierre Habouzit
··O madcoder@debian.org
OOO http://www.madism.org
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 9:34 UTC (permalink / raw)
To: Pierre Habouzit
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
In-Reply-To: <20080731091515.GC12867@artemis.madism.org>
[Pierre, you have the bogus MFT header that people are often complaining
about. Either I have to do extra work to fix the headers, or more people
end up in the To: field. I don't personally care about the latter, but
it seems that others organize their mail based on To versus Cc]
On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:
> --no-walk alters how add_pending_object_with_mode works, which is called
> by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
> --no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
> must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
> or --remote are.
Ah, OK. Then that makes sense, then. And I definitely feel that the
patch I posted is the right thing to do, then. So here is a patch with
commit message. The commit message to patch ratio is ridiculous, but
obviously this fix took some thinking, so it seems best to document it.
-- >8 --
allow "non-option" revision options in parse_option-enabled commands
Commands which use parse_options but also call
setup_revisions must do their parsing in a two step process:
1. first, they parse all options. Anything unknown goes to
parse_revision_opt (which calls handle_revision_opt),
which may claim the option or say "I don't recognize
this"
2. the non-option remainder goes to setup_revisions to
actually get turned into revisions
Some revision options are "non-options" in that they must be
parsed in order with their revision counterparts in
setup_revisions. For example, "--all" functions as a
pseudo-option expanding to all refs, and "--no-walk" affects
refs after it on the command line, but not before. The
revision option parser in step 1 recognizes such options and
sets them aside for later parsing by setup_revisions.
However, the return value used from handle_revision_opt
indicated "I didn't recognize this", which was wrong. It
did, and it took appropriate action (even though that action
was just deferring it for later parsing). Thus it should
return "yes, I recognized this."
Previously, these pseudo-options generated an error when
used with parse_options parsers (currently just blame and
shortlog). With this patch, they should work fine, enabling
things like "git shortlog --all".
Signed-off-by: Jeff King <peff@peff.net>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/revision.c b/revision.c
index a843c42..eaa5572 100644
--- a/revision.c
+++ b/revision.c
@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
!strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
{
unkv[(*unkc)++] = arg;
- return 0;
+ return 1;
}
if (!prefixcmp(arg, "--max-count=")) {
--
1.6.0.rc1.197.gc57ac6
^ permalink raw reply related
* linking libgit.a in C++ projects
From: cte @ 2008-07-31 9:53 UTC (permalink / raw)
To: git
I'm writing a git gui for OS X using cocoa/Objective-C++, and rather
than being lame and parsing the output the various git commands, I'm
using libgit.a to provide all of the needed functionality for my app.
However, the git source uses a few reserved C++ keywords; namely
'typename', and 'new'. So, I was wondering if it is worth submitting a
patch to fix these issues... I'm asking because I'm new to the whole
open source thing, and I don't want to get yelled at by the git
maintainers for submitting stupid patches that no one in their right
mind would accept :)
Thanks!
^ permalink raw reply
* Re: git blame not respecting --find-copies-harder ?
From: Pierre Habouzit @ 2008-07-31 10:22 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, sverre, Björn Steinbrink,
Stephen R. van den Berg, Git Mailinglist
In-Reply-To: <20080731093410.GA21396@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 3074 bytes --]
> [Pierre, you have the bogus MFT header that people are often complaining
> about. Either I have to do extra work to fix the headers, or more people
> end up in the To: field. I don't personally care about the latter, but
> it seems that others organize their mail based on To versus Cc]
[ err if you do a list reply, nobody ends up in To:... but oh well I
can probably remove it, it'll just make my own mutt git folder
awkwardly colorized. ]
On Thu, Jul 31, 2008 at 09:34:10AM +0000, Jeff King wrote:
> On Thu, Jul 31, 2008 at 11:15:15AM +0200, Pierre Habouzit wrote:
> > --no-walk alters how add_pending_object_with_mode works, which is called
> > by handle_revision_arg. IOW, <ref1> --no-walk <ref2> is not the same as
> > --no-walk <ref1> <ref2>. IOW reference arguments and --no-walk ordering
> > must be preserved, IOW --no-walk is a pseudo-argument like --all --tags
> > or --remote are.
>
> Ah, OK. Then that makes sense, then. And I definitely feel that the
> patch I posted is the right thing to do,
Yeah I was convinced the first time I saw it already :)
-- >8 --
allow "non-option" revision options in parse_option-enabled commands
Commands which use parse_options but also call
setup_revisions must do their parsing in a two step process:
1. first, they parse all options. Anything unknown goes to
parse_revision_opt (which calls handle_revision_opt),
which may claim the option or say "I don't recognize
this"
2. the non-option remainder goes to setup_revisions to
actually get turned into revisions
Some revision options are "non-options" in that they must be
parsed in order with their revision counterparts in
setup_revisions. For example, "--all" functions as a
pseudo-option expanding to all refs, and "--no-walk" affects
refs after it on the command line, but not before. The
revision option parser in step 1 recognizes such options and
sets them aside for later parsing by setup_revisions.
However, the return value used from handle_revision_opt
indicated "I didn't recognize this", which was wrong. It
did, and it took appropriate action (even though that action
was just deferring it for later parsing). Thus it should
return "yes, I recognized this."
Previously, these pseudo-options generated an error when
used with parse_options parsers (currently just blame and
shortlog). With this patch, they should work fine, enabling
things like "git shortlog --all".
Signed-off-by: Jeff King <peff@peff.net>
Acked-By: Pierre Habouzit <madcoder@debian.org>
---
revision.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/revision.c b/revision.c
index a843c42..eaa5572 100644
--- a/revision.c
+++ b/revision.c
@@ -1002,7 +1002,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
!strcmp(arg, "--no-walk") || !strcmp(arg, "--do-walk"))
{
unkv[(*unkc)++] = arg;
- return 0;
+ return 1;
}
if (!prefixcmp(arg, "--max-count=")) {
--
1.6.0.rc1.197.gc57ac6
[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]
^ permalink raw reply related
* Re: git blame not respecting --find-copies-harder ?
From: Jeff King @ 2008-07-31 10:33 UTC (permalink / raw)
To: Pierre Habouzit; +Cc: Junio C Hamano, Git Mailinglist
In-Reply-To: <20080731102223.GD12867@artemis.madism.org>
On Thu, Jul 31, 2008 at 12:22:23PM +0200, Pierre Habouzit wrote:
> > [Pierre, you have the bogus MFT header that people are often complaining
> > about. Either I have to do extra work to fix the headers, or more people
> > end up in the To: field. I don't personally care about the latter, but
> > it seems that others organize their mail based on To versus Cc]
>
> [ err if you do a list reply, nobody ends up in To:... but oh well I
> can probably remove it, it'll just make my own mutt git folder
> awkwardly colorized. ]
I can't do a list reply, as mutt sees no list (and I didn't bother
setting up a 'subscribe' variable, since I didn't want to generate MFTs
in the first place). However, I just realized that mutt has an
honor_followup_to, which will do what I want (AFAICT, there is no point
in honoring MFT when using the conventions of this mailing list).
But I wonder if it is as simple in other clients.
-Peff
^ permalink raw reply
* Re: [PATCH] git-svn now work with crlf convertion enabled.
From: Dmitry Potapov @ 2008-07-31 10:45 UTC (permalink / raw)
To: Alexander Litvinov; +Cc: git, Eric Wong
In-Reply-To: <200807311257.49108.litvinov2004@gmail.com>
On Thu, Jul 31, 2008 at 12:57:48PM +0700, Alexander Litvinov wrote:
>
> git-svn fetch files with this patch but I have found that git-svn use
> git-hash-object and provide file name to store into stdin. As far as file is
> a temp file git-hash-object can't correctly apply crlf convertion for the
> file.
It does not look to be true. I did the following test:
mkdir hash_test
cd hash_test
git init
cat <<\=== > hash_test.pl
#!/usr/bin/env perl
use File::Temp qw/tempfile/;
my ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);
print $tmp_fh "Hi\r\n";
$tmp_fh->flush;
system ("echo $tmp_filename | git hash-object --stdin-paths");
===
git config core.autocrlf true
perl hash_test.pl
git config core.autocrlf false
perl hash_test.pl
and the output was
b14df6442ea5a1b382985a6549b85d435376c351
ea6b6afbc2cbed0eb8c0f7561286ab72f349416c
which means that the autocrlf conversion is done for temporary
files created by perl. (I tested it on Linux and Windows/Cygwin).
In any case, I believe the right solution should be adding a
new option to git-hash-object to disable any conversion.
Dmitry
^ permalink raw reply
* Re: [PATCH v3] Advertise the ability to abort a commit
From: Petr Baudis @ 2008-07-31 10:55 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Anders Melchiorsen, git
In-Reply-To: <20080731073609.GA8049@sigill.intra.peff.net>
On Thu, Jul 31, 2008 at 03:36:09AM -0400, Jeff King wrote:
> builtin-commit.c | 19 ++++++++++++-------
> t/t7502-commit.sh | 11 +++++------
> 2 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/builtin-commit.c b/builtin-commit.c
> index f7c053a..b783e6e 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -554,14 +554,18 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
>
> fprintf(fp,
> "\n"
> - "# Please enter the commit message for your changes.\n"
> - "# To abort the commit, use an empty commit message.\n"
> - "# (Comment lines starting with '#' will ");
> + "# Please enter the commit message for your changes.");
> if (cleanup_mode == CLEANUP_ALL)
> - fprintf(fp, "not be included)\n");
> + fprintf(fp,
> + " Lines starting\n"
> + "# with '#' will be ignored, and an empty"
> + " message aborts the commit.\n");
> else /* CLEANUP_SPACE, that is. */
> - fprintf(fp, "be kept.\n"
> - "# You can remove them yourself if you want to)\n");
> + fprintf(fp,
> + " Lines starting\n"
> + "# with '#' will be kept; you may remove them"
> + " yourself if you want to.\n"
> + "# An empty message aborts the commit.\n");
> if (only_include_assumed)
> fprintf(fp, "# %s\n", only_include_assumed);
>
This is rather funny-looking; you print _one_ fragment of the common
string by a common fprintf, but then repeat _second_ fragment of the
still-common string in a per-case fprintf. Can't we at least split this
on the line boundary, if not do something loosely like this?
fprintf(fp,
"\n"
"# Please enter the commit message for your "
"changes. Lines starting\n"
"# with a '#' will be %s "
"and an empty message aborts the commit\n",
cleanup_mode == CLEANUP_ALL ? "ignored,"
/* CLEANUP_SPACE */ : "kept (you may remove them "
"yourself if you want to)\n#");
--
Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name. -- Ken Thompson and Dennis M. Ritchie
^ permalink raw reply
* Re: linking libgit.a in C++ projects
From: Dmitry Potapov @ 2008-07-31 10:57 UTC (permalink / raw)
To: cte; +Cc: git
In-Reply-To: <ac9f0f090807310253v1d97e2a1n4ddf34aa4fdc79f0@mail.gmail.com>
On Thu, Jul 31, 2008 at 02:53:37AM -0700, cte wrote:
> I'm writing a git gui for OS X using cocoa/Objective-C++, and rather
> than being lame and parsing the output the various git commands, I'm
> using libgit.a to provide all of the needed functionality for my app.
Don't do that! libgit.a is an internal library used solely to build
git binaries. It means that its interface can be cahnged at any time.
Though, there is an idea of creating the real git library that other
applications can use, but AFAIK no one is working on it. So parsing
output is the only correct solution right now. In fact, it is not
difficult to do, because most plumbing commands are rather flexibly
in what they output and how.
> However, the git source uses a few reserved C++ keywords; namely
> 'typename', and 'new'.
Because this source code are meant to be compiled by C and not by C++!
Even if we will have real git library for other applications to use,
it still be compiled only by C. Thus, C++ keywords are not issue.
Dmitry
^ permalink raw reply
* Re: [PATCH] 64bit issue in test-parse-options.c
From: Petr Baudis @ 2008-07-31 11:07 UTC (permalink / raw)
To: Pierre Habouzit, H.Merijn Brand, git
In-Reply-To: <20080730140523.GC31392@artemis.madism.org>
On Wed, Jul 30, 2008 at 04:05:23PM +0200, Pierre Habouzit wrote:
> On Wed, Jul 30, 2008 at 12:44:52PM +0000, H.Merijn Brand wrote:
> > On Wed, 30 Jul 2008 14:37:13 +0200, Pierre Habouzit
> > <madcoder@debian.org> wrote:
> > > long is wrong in the first place, parse-opt only uses ints.
> >
> > If I change it to int, the date parsing goes bogus.
>
> That is because OPT_DATE indeed expect a long (why not a time_t I don't
> know btw... but time_t is a long on linux so it doesn't change things a
> lot).
Still, I think converting to time_t for timestamps would be a good
cleanup; I have added it to http://git.or.cz/gitwiki/Janitor.
Petr "Pasky" Baudis
^ permalink raw reply
* Re: [PATCH v3] Advertise the ability to abort a commit
From: Jeff King @ 2008-07-31 11:09 UTC (permalink / raw)
To: Petr Baudis; +Cc: Junio C Hamano, Anders Melchiorsen, git
In-Reply-To: <20080731105539.GM32184@machine.or.cz>
On Thu, Jul 31, 2008 at 12:55:39PM +0200, Petr Baudis wrote:
> > if (cleanup_mode == CLEANUP_ALL)
> > - fprintf(fp, "not be included)\n");
> > + fprintf(fp,
> > + " Lines starting\n"
> > + "# with '#' will be ignored, and an empty"
> > + " message aborts the commit.\n");
> > else /* CLEANUP_SPACE, that is. */
> > - fprintf(fp, "be kept.\n"
> > - "# You can remove them yourself if you want to)\n");
> > + fprintf(fp,
> > + " Lines starting\n"
> > + "# with '#' will be kept; you may remove them"
> > + " yourself if you want to.\n"
> > + "# An empty message aborts the commit.\n");
> > if (only_include_assumed)
> > fprintf(fp, "# %s\n", only_include_assumed);
> >
>
> This is rather funny-looking; you print _one_ fragment of the common
> string by a common fprintf, but then repeat _second_ fragment of the
> still-common string in a per-case fprintf. Can't we at least split this
> on the line boundary, if not do something loosely like this?
I just broke it by sentence, thinking that followed the semantics more
clearly (i.e., the first fprintf says one thing, then the second says
another; however, we must say the second one differently depending on
the case). I almost just split the whole paragraph by cleanup case,
allowing each to be worded and wrapped as most appropriate.
> fprintf(fp,
> "\n"
> "# Please enter the commit message for your "
> "changes. Lines starting\n"
> "# with a '#' will be %s "
> "and an empty message aborts the commit\n",
> cleanup_mode == CLEANUP_ALL ? "ignored,"
> /* CLEANUP_SPACE */ : "kept (you may remove them "
> "yourself if you want to)\n#");
I did something like that before submitting, but decided against it
because:
- I found mine more readable, since it is hard to see in yours exactly
where there will be a linebreak.
- I actually changed the phrasing for the second one. Since we
introduce another clause into the sentence in the CLEANUP_SPACE
case, it makes sense to start another sentence for the final point.
-Peff
^ permalink raw reply
* Re: linking libgit.a in C++ projects
From: cte @ 2008-07-31 11:10 UTC (permalink / raw)
To: Dmitry Potapov; +Cc: git
In-Reply-To: <20080731105727.GF7008@dpotapov.dyndns.org>
On Thu, Jul 31, 2008 at 3:57 AM, Dmitry Potapov <dpotapov@gmail.com> wrote:
> On Thu, Jul 31, 2008 at 02:53:37AM -0700, cte wrote:
>> I'm writing a git gui for OS X using cocoa/Objective-C++, and rather
>> than being lame and parsing the output the various git commands, I'm
>> using libgit.a to provide all of the needed functionality for my app.
>
> Don't do that! libgit.a is an internal library used solely to build
> git binaries. It means that its interface can be cahnged at any time.
> Though, there is an idea of creating the real git library that other
> applications can use, but AFAIK no one is working on it. So parsing
> output is the only correct solution right now. In fact, it is not
> difficult to do, because most plumbing commands are rather flexibly
> in what they output and how.
I'm not worried about the interfaces changing; the gui is tied to a
particular version of git, and I will update the code that calls into
libgit I pull new changes from the mainline into my local clone. Also,
who's to say that the output of the various commands won't change
formats with future releases of git? There is no correct solution if
you are worried about forward compatibility, unless a well defined API
is created (which would be sweet btw, but is probably not a priority).
>> However, the git source uses a few reserved C++ keywords; namely
>> 'typename', and 'new'.
>
> Because this source code are meant to be compiled by C and not by C++!
> Even if we will have real git library for other applications to use,
> it still be compiled only by C. Thus, C++ keywords are not issue.
Clearly ;)
Fortunately, g++ can compile C programs and link static libraries that
were compiled by C compilers, unless of course, they use C++ keywords.
I don't think it is unreasonable to rename the _very few_ C++ keywords
in git's source in the interest of allowing C++ projects to leverage
libgit.
^ permalink raw reply
* Re: linking libgit.a in C++ projects
From: Petr Baudis @ 2008-07-31 11:14 UTC (permalink / raw)
To: Dmitry Potapov; +Cc: cte, git
In-Reply-To: <20080731105727.GF7008@dpotapov.dyndns.org>
On Thu, Jul 31, 2008 at 02:57:27PM +0400, Dmitry Potapov wrote:
> On Thu, Jul 31, 2008 at 02:53:37AM -0700, cte wrote:
> > I'm writing a git gui for OS X using cocoa/Objective-C++, and rather
> > than being lame and parsing the output the various git commands, I'm
> > using libgit.a to provide all of the needed functionality for my app.
>
> Don't do that! libgit.a is an internal library used solely to build
> git binaries. It means that its interface can be cahnged at any time.
I don't think this is that big a problem; there are applications that
are doing this already, e.g. cgit, and if you tie your application to
a particular git version by for example making git a submodule of your
source, this is pretty safe; it will just mean that you will have to
do some non-trivial porting of your code to the new interface each time
you update - but I think large changes in the interface are pretty rare
in practice by now, and there shouldn't be much on the horizon either(?).
> > However, the git source uses a few reserved C++ keywords; namely
> > 'typename', and 'new'.
>
> Because this source code are meant to be compiled by C and not by C++!
> Even if we will have real git library for other applications to use,
> it still be compiled only by C. Thus, C++ keywords are not issue.
What would be the reason to disallow C++ users? The costs aren't that
high, and (modulo, say, extern "C" { }) there should be no C-C++
compatibility issues, right?
--
Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name. -- Ken Thompson and Dennis M. Ritchie
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox