* Re: [PATCH 3/6] fsck: prepare dummy objects for --connectivity-check
From: Junio C Hamano @ 2017-01-17 21:16 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116213204.e7ykwowqzafkexqd@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> + # Drop the index now; we want to be sure that we
> + # recursively notice that we notice the broken objects
> + # because they are reachable from refs, not because
> + # they are in the index.
Rephrase to reduce redundunt "notice"s?
^ permalink raw reply
* Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test
From: Jeff King @ 2017-01-17 20:53 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <xmqqlgu9e7dw.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 12:52:43PM -0800, Junio C Hamano wrote:
> > Since the setup code happens inside a subshell, we can't
> > just set a variable for each object. However, we can stuff
> > all of the sha1s into the $T output variable, which is not
> > used for anything except cleanup.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> > t/t1450-fsck.sh | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Thanks.
>
> It is tempting to move this loop to remove_object, but that is not
> necessary while the user is only this one.
I agree it would be less gross. I avoided it because I knew that I
hacked up remove_object() in the other topic.
-Peff
^ permalink raw reply
* Re: [PATCH 1/6] t1450: clean up sub-objects in duplicate-entry test
From: Junio C Hamano @ 2017-01-17 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116212403.l7ca7crmt47id3mu@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This test creates a multi-level set of trees, but its
> cleanup routine only removes the top-level tree. After the
> test finishes, the inner tree and the blob it points to
> remain, making the inner tree dangling.
>
> A later test ("cleaned up") verifies that we've removed any
> cruft and "git fsck" output is clean. This passes only
> because of a bug in git-fsck which fails to notice dangling
> trees.
>
> In preparation for fixing the bug, let's teach this earlier
> test to clean up after itself correctly. We have to remove
> the inner tree (and therefore the blob, too, which becomes
> dangling after removing that tree).
>
> Since the setup code happens inside a subshell, we can't
> just set a variable for each object. However, we can stuff
> all of the sha1s into the $T output variable, which is not
> used for anything except cleanup.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t1450-fsck.sh | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
Thanks.
It is tempting to move this loop to remove_object, but that is not
necessary while the user is only this one.
>
> diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
> index ee7d4736d..6eef8b28e 100755
> --- a/t/t1450-fsck.sh
> +++ b/t/t1450-fsck.sh
> @@ -189,14 +189,16 @@ test_expect_success 'commit with NUL in header' '
> '
>
> test_expect_success 'tree object with duplicate entries' '
> - test_when_finished "remove_object \$T" &&
> + test_when_finished "for i in \$T; do remove_object \$i; done" &&
> T=$(
> GIT_INDEX_FILE=test-index &&
> export GIT_INDEX_FILE &&
> rm -f test-index &&
> >x &&
> git add x &&
> + git rev-parse :x &&
> T=$(git write-tree) &&
> + echo $T &&
> (
> git cat-file tree $T &&
> git cat-file tree $T
^ permalink raw reply
* Re: [PATCH 2/6] fsck: report trees as dangling
From: Junio C Hamano @ 2017-01-17 20:57 UTC (permalink / raw)
To: Jeff King; +Cc: git, Michael Haggerty, Johannes Schindelin
In-Reply-To: <20170116212535.cohvikwkju5zehr4@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> After checking connectivity, fsck looks through the list of
> any objects we've seen mentioned, and reports unreachable
> and un-"used" ones as dangling. However, it skips any object
> which is not marked as "parsed", as that is an object that
> we _don't_ have (but that somebody mentioned).
>
> Since 6e454b9a3 (clear parsed flag when we free tree
> buffers, 2013-06-05), that flag can't be relied on, and the
> correct method is to check the HAS_OBJ flag. The cleanup in
> that commit missed this callsite, though. As a result, we
> would generally fail to report dangling trees.
>
> We never noticed because there were no tests in this area
> (for trees or otherwise). Let's add some.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
Makes sense, and the new test is very easy to read, too.
Queued; thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Jeff King @ 2017-01-17 20:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqqtw8xe8bp.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 12:32:26PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > That was my general impression, too. But I seem to recall it was you in
> > a nearby thread saying that:
> >
> > if (foo)
> > bar();
> > else {
> > one();
> > two();
> > }
> >
> > was wrong. Maybe I misunderstood.
>
> If it were a new code written like the above, that would have been
> fine. If a new code written with both sides inside {}, that would
> have been fine, too.
>
> IIRC, it was that the original had {} on both, and a patch tried to
> turn that into the above, triggering "why are we churning between
> two acceptable forms?"
Ah, OK. I didn't follow that discussion closely enough to realize that.
-Peff
^ permalink raw reply
* [PATCH] document index_name_pos
From: Stefan Beller @ 2017-01-17 20:46 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
In-Reply-To: <xmqqpojle85c.fsf@gitster.mtv.corp.google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
> These placeholders are meant to encourage those people who dove into
> the code to update it, so from that point of view, I think removing
> it is backwards.
Yes, I am currently understanding and writing up documentation for
index_name_pos. If I recall the latest discussion where we want to have
documentation, I think a quorum favored documentation in the header itself,
c.f. strbuf.h, string-list.h for the most desired state. (Although we do have
Documentation/technical/api-string-list.txt as well ...)
So maybe starting like this?
Thanks,
Stefan
cache.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/cache.h b/cache.h
index 1b67f078dd..e168e4e073 100644
--- a/cache.h
+++ b/cache.h
@@ -575,7 +575,20 @@ extern int verify_path(const char *path);
extern int index_dir_exists(struct index_state *istate, const char *name, int namelen);
extern void adjust_dirname_case(struct index_state *istate, char *name);
extern struct cache_entry *index_file_exists(struct index_state *istate, const char *name, int namelen, int igncase);
+
+/*
+ * Searches for an entry defined by name and namelen in the given index.
+ * If the return value is positive (including 0) it is the position of an
+ * exact match. If the return value is negative, the negated value minus 1 is the
+ * position where the entry would be inserted.
+ * Example: In the current index we have the files a,c,d:
+ * index_name_pos(&index, "a", 1) -> 0
+ * index_name_pos(&index, "b", 1) -> -1
+ * index_name_pos(&index, "c", 1) -> 1
+ * index_name_pos(&index, "d", 1) -> 2
+ */
extern int index_name_pos(const struct index_state *, const char *name, int namelen);
+
#define ADD_CACHE_OK_TO_ADD 1 /* Ok to add */
#define ADD_CACHE_OK_TO_REPLACE 2 /* Ok to replace file/directory */
#define ADD_CACHE_SKIP_DFCHECK 4 /* Ok to skip DF conflict checks */
--
2.11.0.297.g298debce27
^ permalink raw reply related
* Re: [PATCH] documentation: remove unfinished documentation
From: Junio C Hamano @ 2017-01-17 20:36 UTC (permalink / raw)
To: Stefan Beller; +Cc: git
In-Reply-To: <20170117200147.25425-1-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
> When looking for documentation for a specific function, you may be tempted
> to run
>
> git -C Documentation grep index_name_pos
>
> only to find the file technical/api-in-core-index.txt, which doesn't
> help for understanding the given function. It would be better to not find
> these functions in the documentation, such that people directly dive into
> the code instead.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> I run into this a couple of times now, so I put this out tentatively.
These placeholders are meant to encourage those people who dove into
the code to update it, so from that point of view, I think removing
it is backwards.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 20:32 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170117193639.mt3x3md3nbh2qgws@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That was my general impression, too. But I seem to recall it was you in
> a nearby thread saying that:
>
> if (foo)
> bar();
> else {
> one();
> two();
> }
>
> was wrong. Maybe I misunderstood.
If it were a new code written like the above, that would have been
fine. If a new code written with both sides inside {}, that would
have been fine, too.
IIRC, it was that the original had {} on both, and a patch tried to
turn that into the above, triggering "why are we churning between
two acceptable forms?"
^ permalink raw reply
* Re: [RFC] stash --continue
From: Junio C Hamano @ 2017-01-17 20:21 UTC (permalink / raw)
To: Stephan Beyer; +Cc: git
In-Reply-To: <cd784a4e-ee99-564e-81de-9f7f6cc26c67@gmx.net>
Stephan Beyer <s-beyer@gmx.net> writes:
> This led to the idea to have something like "git stash --continue"[1]
> that would expect the user to "git add" the resolved files (as "git
> status" suggests) but then leads to the expected result, i.e. the index
> being the same as before the conflict, the stash being dropped (if "pop"
> was used instead of "apply"), etc.
>
> Likewise, some "git stash --abort"[2] might be useful in case you did
> "git stash pop" with the wrong stash in mind.
>
> What do you think about that?
"git stash pop --continue" (and "git stash apply --continue") would
make quite a lot of sense. I like it very much primarily because it
will give us an opportunity to correct a major UI glitches around
applying stashed changes to the working tree.
Don't people find it strange that "stash pop" that applies cleanly
would not touch the index, leaving (an equivalent of) the changes
stashed earlier floating in the working tree, but "stash pop" that
conflicts and needs a three-way merge touches the index and the
usual way of concluding the manual conflict resolution is "git add"
the paths, meaning that the changes that were not ready hence
floating in the working tree back when the stash was made goes into
the index when the user concludes "stash pop"?
With an explicit "--continue", we can fix that so that we reset the
index to the HEAD. That way, whether the changes have conflict with
the HEAD's tree or not, the end user after "stash pop" will see the
changes in the working tree and "git diff" (no HEAD argument or
"--cached" option) will consistently show what came from the stash.
^ permalink raw reply
* Re: [PATCH] CodingGuidelines: clarify multi-line brace style
From: Jeff King @ 2017-01-17 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqqfukhfpcc.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 11:39:31AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> I think this is pretty clearly the "gray area" mentioned there. Which
> >> yes, does not say "definitely do it this way", but I hope makes it clear
> >> that you're supposed to use judgement about readability.
> >
> > So here's a patch.
> >
> > I know we've usually tried to keep this file to guidelines and not
> > rules, but clearly it has not been clear-cut enough in this instance.
>
> I have two "Huh?" with this patch.
>
> 1. Two exceptions are not treated the same way. The first one is
> "... extends over a few lines, it is excempt from the rule,
> period". The second one, is ambivalent by saying "it can make
> sense", implying that "it may not make sense", so I am not sure
> if this is clarifying that much.
>
> If we want to clarify, perhaps drop "it can make sense to" and
> say
>
> When there are multiple arms to a conditional, and some of
> them require braces, enclose even a single line block in
> braces for consistency.
>
> perhaps?
Yeah. I obviously was adapting the original text, and I think I left too
much of the wishy-washy-ness in. As the point of the patch is to avoid
that, let's take your suggestion. A re-rolled patch is below.
Now the patch is at least self-consistent. The bigger question remains
of: do we want to dictate these rules, or did we prefer the vague
version?
I _thought_ the vague rules were doing fine, but this whole discussion
has made me think otherwise. And I'd just as soon not ever have to
repeat it. ;-/
OTOH, I really do not want to review a bunch of patches that do nothing
but change brace style (and I am sure there is a mix of styles already
in the code base).
> 2. I actually think it is OK to leave some things "gray", but the
> confusion comes when people do not know what to do to things
> that are "gray", and they need a rule for that to be spelled
> out.
>
> When the project says it does not have strong preference
> among multiple choices, you are welcome to write your new
> code using any of them, as long as you are consistent with
> surrounding code. Do not change style of existing code only
> to flip among these styles, though.
>
> That obviously is not limited to the rule/guideline for braces.
The existing document says:
Make your code readable and sensible, and don't try to be clever.
As for more concrete guidelines, just imitate the existing code
(this is a good guideline, no matter which project you are
contributing to). It is always preferable to match the _local_
convention. New code added to Git suite is expected to match
the overall style of existing code. Modifications to existing
code is expected to match the style the surrounding code already
uses (even if it doesn't match the overall style of existing code).
But if you must have a list of rules, here they are.
I guess that is the place to expound on how to interpret the rules. I
dunno. Some of the individual rules that go into "gray areas" already
spell out the "surrounding code" rule.
-Peff
-- >8 --
Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
There are some "gray areas" around when to omit braces from
a conditional or loop body. Since that seems to have
resulted in some arguments, let's be a little more clear
about our preferred style.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
1 file changed, 32 insertions(+), 5 deletions(-)
diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index 4cd95da6b..a4191aa38 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -206,11 +206,38 @@ For C programs:
x = 1;
}
- is frowned upon. A gray area is when the statement extends
- over a few lines, and/or you have a lengthy comment atop of
- it. Also, like in the Linux kernel, if there is a long list
- of "else if" statements, it can make sense to add braces to
- single line blocks.
+ is frowned upon. But there are a few exceptions:
+
+ - When the statement extends over a few lines (e.g., a while loop
+ with an embedded conditional, or a comment). E.g.:
+
+ while (foo) {
+ if (x)
+ one();
+ else
+ two();
+ }
+
+ if (foo) {
+ /*
+ * This one requires some explanation,
+ * so we're better off with braces to make
+ * it obvious that the indentation is correct.
+ */
+ doit();
+ }
+
+ - When there are multiple arms to a conditional and some of them
+ require braces, enclose even a single line block in braces for
+ consistency. E.g.:
+
+ if (foo) {
+ doit();
+ } else {
+ one();
+ two();
+ three();
+ }
- We try to avoid assignments in the condition of an "if" statement.
--
2.11.0.651.g41f4a5c4e
^ permalink raw reply related
* [PATCH] documentation: remove unfinished documentation
From: Stefan Beller @ 2017-01-17 20:01 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
When looking for documentation for a specific function, you may be tempted
to run
git -C Documentation grep index_name_pos
only to find the file technical/api-in-core-index.txt, which doesn't
help for understanding the given function. It would be better to not find
these functions in the documentation, such that people directly dive into
the code instead.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
I run into this a couple of times now, so I put this out tentatively.
Thanks,
Stefan
Documentation/technical/api-in-core-index.txt | 21 ---------------------
1 file changed, 21 deletions(-)
delete mode 100644 Documentation/technical/api-in-core-index.txt
diff --git a/Documentation/technical/api-in-core-index.txt b/Documentation/technical/api-in-core-index.txt
deleted file mode 100644
index adbdbf5d75..0000000000
--- a/Documentation/technical/api-in-core-index.txt
+++ /dev/null
@@ -1,21 +0,0 @@
-in-core index API
-=================
-
-Talk about <read-cache.c> and <cache-tree.c>, things like:
-
-* cache -> the_index macros
-* read_index()
-* write_index()
-* ie_match_stat() and ie_modified(); how they are different and when to
- use which.
-* index_name_pos()
-* remove_index_entry_at()
-* remove_file_from_index()
-* add_file_to_index()
-* add_index_entry()
-* refresh_index()
-* discard_index()
-* cache_tree_invalidate_path()
-* cache_tree_update()
-
-(JC, Linus)
--
2.11.0.297.g298debce27
^ permalink raw reply related
* Re: [PATCH] Documentation/bisect: improve on (bad|new) and (good|bad)
From: Junio C Hamano @ 2017-01-17 19:58 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Christian Couder, git, Manuel Ullmann, Christian Couder
In-Reply-To: <vpqfukjpdnp.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>> But what if bad-A and bad-B have more than one merge bases? We
>> won't know which side the badness came from.
>>
>> o---o---o---bad-A
>> / \ /
>> -----Good---o---o---o /
>> \ / \
>> o---o---o---bad-B
>>
>> Being able to bisect the region of DAG bound by "^Good bad-A bad-B"
>> may have value in such a case. I dunno.
>
> I could help finding several guilty commits, but anyway you can't
> guarantee you'll find them all as soon as you use a binary search: if
> the history looks like
>
> --- Good --- Bad --- Good --- Good --- Bad --- Good --- Bad
>
> then without examining all commits, you can't tell how many good->bad
> switches occured.
>
> But keeping several bad commits wouldn't help keeping the set of
> potentially guilty commits small: bad commits appear on the positive
> side in "^Good bad-A bad-B", so having more bad commits mean having a
> larger DAG to explore (which is a bit counter-intuitive: without
> thinking about it I'd have said "more info => less commits to explore").
>
> So, if finding all guilty commits is not possible, I'm not sure how
> valuable it is to try to find several of them.
The criss-cross merge example, is not trying to find multiple
sources of badness. It still assumes [*1*] that there is only one
event that introduced the badness observed at bad-A and bad-B, both
of which inherited the badness from the same such event. Unlike a
case with a single/unique merge-base, we cannot say "we can start
from the merge-base, as their common badness must be coming from the
same place". The badness may exist in the first 'o' on the same
line as bad-A in the above picture, which is an ancestor of one
merge-base on that line and does not break the other merge base on
the same line as bad-B, for example.
> OTOH, keeping several good commits is needed to find a commit for which
> all parents are good and the commit is bad.
Yes, that is correct.
[Footnote]
*1* The assumption is what makes "bisect" workable. If the
assumption does not hold, then "bisect" would not give a useful
answer "where did I screw up?". It gives a fairly useless "I
found one bad commit whose parent is good---there is no
guarantee if that has anything to do with the badness you are
seeing at the tip".
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 19:45 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1701161226090.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
>
> On Sun, 15 Jan 2017, Junio C Hamano wrote:
>
>> * js/prepare-sequencer-more (2017-01-09) 38 commits
>
> I think that it adds confusion rather than value to specifically use a
> different branch name than I indicated in my submission, unless there is a
> really good reason to do so (which I fail to see here).
I've been meaning to rename it to match yours, for the exact reason.
The only reason was I needed my time spent to deal with other
topics, and reusing the same topic name as I used very first time
was less work. I'll find time to update it (if you are curious, it
is not just "git branch -m").
Thanks.
> The only outstanding review comments I know about are your objection to
> the name of the read_env_script() function (which I shot down as bogus),
Not the "name", but the implementation not sharing the same code
with "am" codepath even though they originate from the same shell
function and serve the same purpose.
> and the rather real bug fix I sent out as a fixup! which you may want to
> squash in (in the alternative, I can mailbomb v4 of the entire sequencer-i
> patch series, that is your choice).
I'd rather see the "make elengant" thing redone in a more sensible
way, but I feel that it is waste of my time to help you see the
distinction between maintainable code and code that happens to work
with today's requirement, so I give up, hoping that other people
will later refactor the code that should be shared after the series
lands. I'll squash the fixup! thing in, and as I already said a few
days ago, I do not think we'd want v4 (rather we'd want to go
incremental).
^ permalink raw reply
* Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend
From: Junio C Hamano @ 2017-01-17 19:50 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Kevin Daudt, Dennis Kaarsemaker, Stephan Beyer, Jeff King
In-Reply-To: <alpine.DEB.2.20.1701161129240.3469@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > The original code is:
>> >
>> > . "$author_script"
>>
>> [...]
>>
>> If the code in the sequencer.c reads things other than the three
>> variables we ourselves set, and make them into environment variables
>> and propagate to subprocesses (hooks and editors), it would be a
>> bug. The original did not intend to do that (the dot-sourcing is
>> overly loose than reading three known variables and nothing else,
>> but is OK because we do not support the case where end users muck
>> with the file). Also, writing FOO=BAR alone (not "export FOO=BAR"
>> or "FOO=BAR; export FOO") to the file wouldn't have exported FOO to
>> subprocesses anyway.
>
> That analysis cannot be completely correct, as the GIT_AUTHOR_* variables
> *are* used by the `git commit` subprocess.
In the scripted version, do_with_author shell function explicitly
exports GIT_AUTHOR_* variables because they are the only ones we
care about that are read from existing commit and carried forward
via the author-script mechanism. We do not care about, and in fact
we do not intend to support, any other variables or shell commands
that appear in the author-script.
^ permalink raw reply
* Re: [PATCH] CodingGuidelines: clarify multi-line brace style
From: Junio C Hamano @ 2017-01-17 19:39 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170116220821.4tji5mrfcdbdpfuo@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I think this is pretty clearly the "gray area" mentioned there. Which
>> yes, does not say "definitely do it this way", but I hope makes it clear
>> that you're supposed to use judgement about readability.
>
> So here's a patch.
>
> I know we've usually tried to keep this file to guidelines and not
> rules, but clearly it has not been clear-cut enough in this instance.
I have two "Huh?" with this patch.
1. Two exceptions are not treated the same way. The first one is
"... extends over a few lines, it is excempt from the rule,
period". The second one, is ambivalent by saying "it can make
sense", implying that "it may not make sense", so I am not sure
if this is clarifying that much.
If we want to clarify, perhaps drop "it can make sense to" and
say
When there are multiple arms to a conditional, and some of
them require braces, enclose even a single line block in
braces for consistency.
perhaps?
2. I actually think it is OK to leave some things "gray", but the
confusion comes when people do not know what to do to things
that are "gray", and they need a rule for that to be spelled
out.
When the project says it does not have strong preference
among multiple choices, you are welcome to write your new
code using any of them, as long as you are consistent with
surrounding code. Do not change style of existing code only
to flip among these styles, though.
That obviously is not limited to the rule/guideline for braces.
> -- >8 --
> Subject: [PATCH] CodingGuidelines: clarify multi-line brace style
>
> There are some "gray areas" around when to omit braces from
> a conditional or loop body. Since that seems to have
> resulted in some arguments, let's be a little more clear
> about our preferred style.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Documentation/CodingGuidelines | 37 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 32 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
> index 4cd95da6b..0e336e99d 100644
> --- a/Documentation/CodingGuidelines
> +++ b/Documentation/CodingGuidelines
> @@ -206,11 +206,38 @@ For C programs:
> x = 1;
> }
>
> - is frowned upon. A gray area is when the statement extends
> - over a few lines, and/or you have a lengthy comment atop of
> - it. Also, like in the Linux kernel, if there is a long list
> - of "else if" statements, it can make sense to add braces to
> - single line blocks.
> + is frowned upon. But there are a few exceptions:
> +
> + - When the statement extends over a few lines (e.g., a while loop
> + with an embedded conditional, or a comment). E.g.:
> +
> + while (foo) {
> + if (x)
> + one();
> + else
> + two();
> + }
> +
> + if (foo) {
> + /*
> + * This one requires some explanation,
> + * so we're better off with braces to make
> + * it obvious that the indentation is correct.
> + */
> + doit();
> + }
> +
> + - When there are multiple arms to a conditional, it can make
> + sense to add braces to single line blocks for consistency.
> + E.g.:
> +
> + if (foo) {
> + doit();
> + } else {
> + one();
> + two();
> + three();
> + }
>
> - We try to avoid assignments in the condition of an "if" statement.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Jeff King @ 2017-01-17 19:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <xmqqk29tfq79.fsf@gitster.mtv.corp.google.com>
On Tue, Jan 17, 2017 at 11:20:58AM -0800, Junio C Hamano wrote:
> > Documentation/CodingGuidelines says:
> >
> > - We avoid using braces unnecessarily. I.e.
> >
> > if (bla) {
> > x = 1;
> > }
> >
> > is frowned upon. A gray area is when the statement extends
> > over a few lines, and/or you have a lengthy comment atop of
> > it. Also, like in the Linux kernel, if there is a long list
> > of "else if" statements, it can make sense to add braces to
> > single line blocks.
> >
> > I think this is pretty clearly the "gray area" mentioned there. Which
> > yes, does not say "definitely do it this way", but I hope makes it clear
> > that you're supposed to use judgement about readability.
>
> I always took "gray area" to mean "we do not have strong preference
> either way, i.e.
>
> * It is OK for you to write your new code in either style (the
> usual "match existing style in surrounding code" applies,
> obviously);
>
> * It is not OK for you to churn the codebase with a patch that only
> changes existing code to flip between the two styles.
That was my general impression, too. But I seem to recall it was you in
a nearby thread saying that:
if (foo)
bar();
else {
one();
two();
}
was wrong. Maybe I misunderstood.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 19:20 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Sixt, git
In-Reply-To: <20170116220014.bwi5xi2br56lyqsw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Documentation/CodingGuidelines says:
>
> - We avoid using braces unnecessarily. I.e.
>
> if (bla) {
> x = 1;
> }
>
> is frowned upon. A gray area is when the statement extends
> over a few lines, and/or you have a lengthy comment atop of
> it. Also, like in the Linux kernel, if there is a long list
> of "else if" statements, it can make sense to add braces to
> single line blocks.
>
> I think this is pretty clearly the "gray area" mentioned there. Which
> yes, does not say "definitely do it this way", but I hope makes it clear
> that you're supposed to use judgement about readability.
I always took "gray area" to mean "we do not have strong preference
either way, i.e.
* It is OK for you to write your new code in either style (the
usual "match existing style in surrounding code" applies,
obviously);
* It is not OK for you to churn the codebase with a patch that only
changes existing code to flip between the two styles.
^ permalink raw reply
* Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)
From: Junio C Hamano @ 2017-01-17 19:17 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Sixt, Johannes Schindelin, git
In-Reply-To: <20170116214411.a6wnp66vxydmpmgw@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Jan 16, 2017 at 09:33:07PM +0100, Johannes Sixt wrote:
>
>> However, Jeff's patch is intended to catch exactly these cases (not for the
>> cases where this happens accidentally, but when they happen with malicious
>> intent).
>>
>> We are talking about user-provided data that is reproduced by die() or
>> error(). I daresay that we do not have a single case where it is intended
>> that this data is intentionally multi-lined, like a commit message. It can
>> only be an accident or malicious when it spans across lines.
>>
>> I know we allow CR and LF in file names, but in all cases where such a name
>> appears in an error message, it is *not important* that the data is
>> reproduced exactly. On the contrary, it is usually more helpful to know that
>> something strange is going on. The question marks are a strong indication to
>> the user for this.
>
> Yes, exactly. Thanks for explaining this better than I obviously was
> doing. :)
Yup. In the "funny filename" case, the updated code indeed is doing
the right thing. So one remaining possible issue is if we ourselves
deliberately feed a raw CR (or any non-filtered control code) to
error() and friends because their native effect (like returning the
carriage to overwrite what we have already said with the remainder
of the message by using CR) to functions that go through vreportf()
interface. I personally do not think there is any---the progress
meter code does use CR for that but they don't do vreportf().
I do not think we write "message\r\n" even for Windows; we rely on
vfprintf() and other stdio layer to turn "message\n" into
"message\r\n" before it hits write(2) or its equivalent? So I
understand that this should affect LF and CRLF systems the same way
(meaning, no negative effect).
So... can we move this forward?
^ permalink raw reply
* Re: [BUG/RFC] Git Gui: GIT_DIR leaks to spawned Git Bash
From: Max Kirillov @ 2017-01-17 18:42 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Max Kirillov, Pat Thoyts, git, Junio C Hamano
In-Reply-To: <alpine.DEB.2.20.1701171145050.3469@virtualbox>
On Tue, Jan 17, 2017 at 11:52:29AM +0100, Johannes Schindelin wrote:
> Hi Max,
>
> On Tue, 17 Jan 2017, Max Kirillov wrote:
>
>> Apparently various GIT_* environment variables (most
>> interesting is GIT_DIR but AFAIR there were more) leak to
>> shell session launched from Git Gui's "Git Bash" menu item.
...
> After all, you won't even notice unless you use the very
> same Git Bash to navigate to a *different* worktree.
That's right, I haven't been noticing it myself for quite a
time. But once I decided to tweak my habits to having only
one terminal window opened, and immediately noticed.
> And having said *that*, I have to admit that I was unable to reproduce
I have found exact way to reproduce it and actually the
reason. Turns out to be obvious mistake in code. PR is in
github: https://github.com/git-for-windows/git/pull/1032
--
Max
^ permalink raw reply
* Re: submodule network operations [WAS: Re: [RFC/PATCH 0/4] working tree operations: support superprefix]
From: Stefan Beller @ 2017-01-17 18:43 UTC (permalink / raw)
To: Brian J. Davis; +Cc: Brandon Williams, git@vger.kernel.org, David Turner
In-Reply-To: <ebf6c90e-044f-5538-2325-601d002a81fe@gmail.com>
On Sun, Jan 15, 2017 at 1:02 PM, Brian J. Davis <bitminer@gmail.com> wrote:
>>
>> Technically it is submodule.<name>.url instead of
>> submodule.<path>.url. The name is usually the path initially, and once
>> you move the submodule, only the path changes, the name is supposed to
>> be constant and stay the same.
>
> I am not certain what is meant by this. All I know is I can use my
> "directory_to_checkout" above to place in tree relative from root the
> project any where in the tree not already tracked by git. You state name
> instead of path, but it allows path correct? Either that or I have gone off
> reservation with my use of git for years now. Maybe this is a deviation from
> how it is documented/should work and how it actually works? It works great
> how I use it.
Yes name can equal the path (and usually does). This is a minor detail
that is only relevant for renaming submodules, so ... maybe let's not
focus on it too much. :)
>>>
>>>
>>> but if say I want to pull from some server 2 and do a
>>>
>>> git submodule update --init --recursive
>>
>> That is why the "git submodule init" exists at all.
>>
>> git submodule init
>> $EDIT .git/config # change submodule.<name>.url to server2
>> git submodule update # that uses the adapted url and
>> # creates the submodule repository.
>>
>> # From now on the submodule is on its own.
>> cd <submodule> && git config --list
>> # prints an "origin" remote and a lot more
>>
>> For a better explanation, I started a documentation series, see
>>
>> https://github.com/gitster/git/commit/e2b51b9df618ceeff7c4ec044e20f5ce9a87241e
>>
>> (It is not finished, but that is supposed to explain this exact pain
>> point in the
>> STATES section, feel free to point out missing things or what is hard
>> to understand)
>
> I am not sure I got much out of the STATES section regarding my problem.
Your original problem as far as I understand is this:
You have a project with submodules.
The submodules are described in the .gitmodules file.
But the URL is pointing to an undesired location.
You want to rewrite the URLs before actually cloning the submodules.
And to solve this problem we need to perform multiple steps:
# --no is the default, just for clarity here:
git clone <project> --no-recurse-submodules
# The submodules are now in the *uninitialized* state
git submodule init
# the submodules are in the initialized state
git submodule update
# submodules are populated, i.e. cloned from
# the configured URLs and put into the working tree at
# the appropriate path.
Between the init and the update step you can modify the URLs.
These commands are just a repetition from the first email, but the
git commands can be viewed as moving from one state to another
for submodules; submodules itself can be seen as a state machine
according to that proposed documentation. Maybe such a state machine
makes it easier to understand for some people.
>>> what I would get is from someserver1 not someserver2 as there is no
>>> "origin"
>>> support for submodules. Is this correct? If so can origin support be
>>> added
>>> to submodules?
>>
>> Can you explain more in detail what you mean by origin support?
>
> Yes so when we do a:
>
> git push origin master
>
> origin is of course the Remote (Remotes
> https://git-scm.com/book/en/v2/Git-Basics-Working-with-Remotes)
>
> So I best use terminology "Remotes" support. Git push supports remotes, but
> git submodules does not. The basic idea is this:
>
> If Git allowed multiple submodule
> (https://git-scm.com/book/en/v2/Git-Tools-Submodules) Remotes to be
> specified say as an example:
>
> git submodule add [remote] [url]
>
> git submodule add origin https://github.com/chaconinc/DbConnector
> git submodule add indhouse https://indhouse .corp/chaconinc/DbConnector
>
> Where:
>
> [submodule "DbConnector"]
> path = DbConnector
> url = https://github.com/chaconinc/DbConnector
>
> Could then change to:
>
> [submodule "DbConnector"]
> path = DbConnector
> remote.origin.url = https://github.com/chaconinc/DbConnector
> remote.origin.url = https://indhouse .corp/chaconinc/DbConnector
here I assume there is a typo and the second remote.origin.url should be
remote.inhouse.url ?
>
>
> Then it should be possible to get:
>
> git submodules update --init --recursive
which would setup the submodule with both
[remote "origin"]
url = https://github.com/..
[remote "inhouse"]
url = https://inhouse.corp/..
But where do we clone it from?
(Or do we just do a "git init" on that submodule and fetch
from both remotes? in which order?)
>
> To support
>
> git submodules update [remote] --init --recursive
This would just clone/fetch from the specified remote?
If implementing this, we may run into a collision with the
specified submodules, what if a submodule is at
path "origin" ?
Does "git submodule update origin --init --recursive"
then mean to update the single "origin" submodule or
all submodules from their origin remote?
>
> And thus allow
>
> git submodules update origin --init --recursive
>
> git submodules update indhouse --init --recursive
understood. I like the idea of being able to specify
multiple remotes from the superproject..
^ permalink raw reply
* Re: [RFC] Add support for downloading blobs on demand
From: Jeff King @ 2017-01-17 18:42 UTC (permalink / raw)
To: Ben Peart; +Cc: git, benpeart
In-Reply-To: <20170113155253.1644-1-benpeart@microsoft.com>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=utf-8, Size: 9263 bytes --]
This is an issue I've thought a lot about. So apologies in advance that
this response turned out a bit long. :)
On Fri, Jan 13, 2017 at 10:52:53AM -0500, Ben Peart wrote:
> Design
> ~~~~~~
>
> Clone and fetch will pass a “--lazy-clone” flag (open to a better name
> here) similar to “--depth” that instructs the server to only return
> commits and trees and to ignore blobs.
>
> Later during git operations like checkout, when a blob cannot be found
> after checking all the regular places (loose, pack, alternates, etc),
> git will download the missing object and place it into the local object
> store (currently as a loose object) then resume the operation.
Have you looked at the "external odb" patches I wrote a while ago, and
which Christian has been trying to resurrect?
http://public-inbox.org/git/20161130210420.15982-1-chriscool@tuxfamily.org/
This is a similar approach, though I pushed the policy for "how do you
get the objects" out into an external script. One advantage there is
that large objects could easily be fetched from another source entirely
(e.g., S3 or equivalent) rather than the repo itself.
The downside is that it makes things more complicated, because a push or
a fetch now involves three parties (server, client, and the alternate
object store). So questions like "do I have all the objects I need" are
hard to reason about.
If you assume that there's going to be _some_ central Git repo which has
all of the objects, you might as well fetch from there (and do it over
normal git protocols). And that simplifies things a bit, at the cost of
being less flexible.
> To prevent git from accidentally downloading all missing blobs, some git
> operations are updated to be aware of the potential for missing blobs.
> The most obvious being check_connected which will return success as if
> everything in the requested commits is available locally.
Actually, Git is pretty good about trying not to access blobs when it
doesn't need to. The important thing is that you know enough about the
blobs to fulfill has_sha1_file() and sha1_object_info() requests without
actually fetching the data.
So the client definitely needs to have some list of which objects exist,
and which it _could_ get if it needed to.
The one place you'd probably want to tweak things is in the diff code,
as a single "git log -Sfoo" would fault in all of the blobs.
> To minimize the impact on the server, the existing dumb HTTP protocol
> endpoint “objects/<sha>” can be used to retrieve the individual missing
> blobs when needed.
This is going to behave badly on well-packed repositories, because there
isn't a good way to fetch a single object. The best case (which is not
implemented at all in Git) is that you grab the pack .idx, then grab
"slices" of the pack corresponding to specific objects, including
hunting down delta bases.
But then next time the server repacks, you have to throw away your .idx
file. And those can be big. The .idx for linux.git is 135MB. You really
wouldn't want to do an incremental fetch of 1MB worth of objects and
have to grab the whole .idx just to figure out which bytes you needed.
You can solve this by replacing the dumb-http server with a smart one
that actually serves up the individual objects as if they were truly
sitting on the filesystem. But then you haven't really minimized impact
on the server, and you might as well teach the smart protocols to do
blob fetches.
One big hurdle to this approach, no matter the protocol, is how you are
going to handle deltas. Right now, a git client tells the server "I have
this commit, but I want this other one". And the server knows which
objects the client has from the first, and which it needs from the
second. Moreover, it knows that it can send objects in delta form
directly from disk if the other side has the delta base.
So what happens in this system? We know we don't need to send any blobs
in a regular fetch, because the whole idea is that we only send blobs on
demand. So we wait for the client to ask us for blob A. But then what do
we send? If we send the whole blob without deltas, we're going to waste
a lot of bandwidth.
The on-disk size of all of the blobs in linux.git is ~500MB. The actual
data size is ~48GB. Some of that is from zlib, which you get even for
non-deltas. But the rest of it is from the delta compression. I don't
think it's feasible to give that up, at least not for "normal" source
repos like linux.git (more on that in a minute).
So ideally you do want to send deltas. But how do you know which objects
the other side already has, which you can use as a delta base? Sending
the list of "here are the blobs I have" doesn't scale. Just the sha1s
start to add up, especially when you are doing incremental fetches.
I think this sort of things performs a lot better when you just focus on
large objects. Because they don't tend to delta well anyway, and the
savings are much bigger by avoiding ones you don't want. So a directive
like "don't bother sending blobs larger than 1MB" avoids a lot of these
issues. In other words, you have some quick shorthand to communicate
between the client and server: this what I have, and what I don't.
Normal git relies on commit reachability for that, but there are
obviously other dimensions. The key thing is that both sides be able to
express the filters succinctly, and apply them efficiently.
> After cloning, the developer can use sparse-checkout to limit the set of
> files to the subset they need (typically only 1-10% in these large
> repos). This allows the initial checkout to only download the set of
> files actually needed to complete their task. At any point, the
> sparse-checkout file can be updated to include additional files which
> will be fetched transparently on demand.
If most of your benefits are not from avoiding blobs in general, but
rather just from sparsely populating the tree, then it sounds like
sparse clone might be an easier path forward. The general idea is to
restrict not just the checkout, but the actual object transfer and
reachability (in the tree dimension, the way shallow clone limits it in
the time dimension, which will require cooperation between the client
and server).
So that's another dimension of filtering, which should be expressed
pretty succinctly: "I'm interested in these paths, and not these other
ones." It's pretty easy to compute on the server side during graph
traversal (though it interacts badly with reachability bitmaps, so there
would need to be some hacks there).
It's an idea that's been talked about many times, but I don't recall
that there were ever working patches. You might dig around in the list
archive under the name "sparse clone" or possibly "narrow clone".
> Now some numbers
> ~~~~~~~~~~~~~~~~
>
> One repo has 3+ million files at tip across 500K folders with 5-6K
> active developers. They have done a lot of work to remove large files
> from the repo so it is down to < 100GB.
>
> Before changes: clone took hours to transfer the 87GB .pack + 119MB .idx
>
> After changes: clone took 4 minutes to transfer 305MB .pack + 37MB .idx
>
> After hydrating 35K files (the typical number any individual developer
> needs to do their work), there was an additional 460 MB of loose files
> downloaded.
It sounds like you have a case where the repository has a lot of large
files that are either historical, or uninteresting the sparse-tree
dimension.
How big is that 460MB if it were actually packed with deltas?
> Future Work
> ~~~~~~~~~~~
>
> The current prototype calls a new hook proc in sha1_object_info_extended
> and read_object, to download each missing blob. A better solution would
> be to implement this via a long running process that is spawned on the
> first download and listens for requests to download additional objects
> until it terminates when the parent git operation exits (similar to the
> recent long running smudge and clean filter work).
Yeah, see the external-odb discussion. Those prototypes use a process
per object, but I think we all agree after seeing how the git-lfs
interface has scaled that this is a non-starter. Recent versions of
git-lfs do the single-process thing, and I think any sort of
external-odb hook should be modeled on that protocol.
> Need to investigate an alternate batching scheme where we can make a
> single request for a set of "related" blobs and receive single a
> packfile (especially during checkout).
I think this sort of batching is going to be the really hard part to
retrofit onto git. Because you're throwing out the procedural notion
that you can loop over a set of objects and ask for each individually.
You have to start deferring computation until answers are ready. Some
operations can do that reasonably well (e.g., checkout), but something
like "git log -p" is constantly digging down into history. I suppose you
could just perform the skeleton of the operation _twice_, once to find
the list of objects to fault in, and the second time to actually do it.
That will make git feel a lot slower, because a lot of the illusion of
speed is the way it streams out results. OTOH, if you have to wait to
fault in objects from the network, it's going to feel pretty slow
anyway. :)
-Peff
^ permalink raw reply
* Re: gitk pull request // was: Re: gitk: "lime" color incompatible with older Tk versions
From: Junio C Hamano @ 2017-01-17 18:34 UTC (permalink / raw)
To: Paul Mackerras
Cc: David Aguilar, Stefan Beller, Andrew Janke, git@vger.kernel.org
In-Reply-To: <xmqqy3yba1jg.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Paul Mackerras <paulus@ozlabs.org> writes:
>
>>> Paul, is it a good time to pull, or do you still have something not
>>> published yet that should go together with what you have already
>>> queued?
>>
>> I recently pushed out one more commit to update the Russian
>> translation from Dimitriy Ryazantcev. The head is now 8fef3f36b779.
>> I have a couple more series that I am currently reviewing, but nothing
>> immediately ready to publish. It would be a good time for you to do a
>> pull, since the "lime" color fix and the memory consumption fixes
>> should be helpful for a lot of people.
>
> Thanks. I did want to get the memory consumption fix sooner rather
> than later, and this is very much appreciated.
>
> Pulled.
Hmph. I am getting these:
SUBDIR gitk-git
Generating catalog po/sv.msg
msgfmt --statistics --tcl po/sv.po -l sv -d po/
po/sv.po:1388: duplicate message definition...
po/sv.po:380: ...this is the location of the first definition
msgfmt: found 1 fatal error
make[1]: *** [po/sv.msg] Error 1
make: *** [all] Error 2
Anybody else see this?
^ permalink raw reply
* Re: [PATCH v5 5/7] builtin/tag: add --format argument for tag -v
From: Santiago Torres @ 2017-01-17 17:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173239.ir6jxbz46vwwzoht@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 301 bytes --]
> VERBOSE|QUIET _does_ have a meaning, which is "show the payload, but do
> not print the signature buffer". Perhaps just renaming QUIET to
> OMIT_STATUS or something would make it more clear.
>
Let me give this a go too. OMIT_STATUS does sound less confusing.
Thanks,
-Santiago.
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Santiago Torres @ 2017-01-17 17:33 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173003.sgipqc2cijpdrukb@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]
Yeah, this actually looks more cleaner.
Let me give it a go.
Thanks!
-Santiago.
On Tue, Jan 17, 2017 at 12:30:04PM -0500, Jeff King wrote:
> On Tue, Jan 17, 2017 at 12:25:31PM -0500, Jeff King wrote:
>
> > Actually, looking at the callsites, I think they are fine to just call
> > pretty_print_ref() themselves, and I don't think it actually matters if
> > it happens before or after the verification.
>
> Oh, sorry, I misread it. We do indeed early-return from the verification
> and skip the printing in that case. So it'd be more like:
>
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9da11e0c2..068f392b6 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
> if (fmt_pretty)
> flags = GPG_VERIFY_QUIET;
>
> - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> + if (gpg_verify_tag(sha1, ref, flags))
> + return -1;
> +
> + pretty_print_ref(name, sha1, fmt_pretty);
> + return 0;
> }
>
> static int do_sign(struct strbuf *buffer)
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 212449f47..b3f08f705 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -58,10 +58,19 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
> while (i < argc) {
> unsigned char sha1[20];
> const char *name = argv[i++];
> - if (get_sha1(name, sha1))
> +
> + if (get_sha1(name, sha1)) {
> had_error = !!error("tag '%s' not found.", name);
> - else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
> + continue;
> + }
> +
> + if (gpg_verify_tag(sha1, name, flags)) {
> had_error = 1;
> + continue;
> + }
> +
> + if (fmt_pretty)
> + pretty_print_ref(name, sha1, fmt_pretty);
> }
> return had_error;
> }
>
> which I think is still an improvement (the printing, rather than being
> an obscure parameter to the overloaded verify_and_format_tag()
> function, is clearly a first-class operation).
>
> -Peff
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v5 3/7] tag: add format specifier to gpg_verify_tag
From: Jeff King @ 2017-01-17 17:34 UTC (permalink / raw)
To: Santiago Torres; +Cc: git, gitster, sunshine, walters, Lukas Puehringer
In-Reply-To: <20170117173346.paqrsroauoskxpn6@LykOS.localdomain>
On Tue, Jan 17, 2017 at 12:33:46PM -0500, Santiago Torres wrote:
> Yeah, this actually looks more cleaner.
>
> Let me give it a go.
Neat. Note there's a bug:
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9da11e0c2..068f392b6 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > @@ -114,7 +114,11 @@ static int verify_tag(const char *name, const char *ref,
> > if (fmt_pretty)
> > flags = GPG_VERIFY_QUIET;
> >
> > - return verify_and_format_tag(sha1, ref, fmt_pretty, flags);
> > + if (gpg_verify_tag(sha1, ref, flags))
> > + return -1;
> > +
> > + pretty_print_ref(name, sha1, fmt_pretty);
> > + return 0;
The final print should be guarded by "if (fmt_pretty)".
-Peff
^ 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