Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren  writes:

>>  - allow callers to align 1st prefix (e.g. "error: ") with the
>>leading indent for the second and subsequent lines by passing the
>>second prefix with appropriate display width.
>
> I suspect this second prefix would always consist of
> strlen(first_prefix) spaces? We should be able to construct it on the
> fly, without any need for manual counting and human mistakes.

I wouldn't be so bold to claim that, given especially that we are
talking about i18n/l10n where byte count, character count and
display width are all different even on a terminal with fixed-width
font.

[PATCH v5 0/2] blame and log: prevent error if range ends past end of file

[istephens]

Okay, I think that sounds reasonable. I've amended the patch so that the
line number is clipped to 1 when reading in a line range. And -L,-n is now
treated the same as -L1,-n i.e. it will blame (or log) the first line of the
file only.

[PATCH] blame: prevent error if range ends past end of file

[istephens]

From: Isabella Stephens 

If the -L option is used to specify a line range in git blame, and the
end of the range is past the end of the file, git will fail with a fatal
error. This commit prevents such behavior - instead we display the blame
for existing lines within the specified range. Tests and documentation
are ammended accordingly.

This commit also fixes two corner cases. Blaming -L n,-(n+1) now blames
the first n lines of a file rather than from n to the end of the file.
Blaming -L ,-n will be treated as -L 1,-n and blame the first line of
the file, rather than blaming the whole file.

Signed-off-by: Isabella Stephens 
---
 Documentation/git-blame.txt   | 10 ++
 builtin/blame.c   |  4 ++--
 line-range.c  |  2 +-
 t/t8003-blame-corner-cases.sh | 12 
 4 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index 16323eb80..8cb81f57a 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -152,6 +152,16 @@ Also you can use a regular expression to specify the line 
range:
 
 which limits the annotation to the body of the `hello` subroutine.
 
+A range that begins or ends outside the bounds of the file will
+blame the relevant lines. For example:
+
+   git blame -L 10,-20 foo
+   git blame -L 10,+20 foo
+
+will respectively blame the first 10 and last 11 lines of a
+20 line file. However, blaming a line range that is entirely
+outside the bounds of the file will fail.
+
 When you are not interested in changes older than version
 v2.6.18, or changes older than 3 weeks, you can use revision
 range specifiers  similar to 'git rev-list':
diff --git a/builtin/blame.c b/builtin/blame.c
index 9dcb367b9..e1359b192 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -886,13 +886,13 @@ int cmd_blame(int argc, const char **argv, const char 
*prefix)
nth_line_cb, , lno, anchor,
, , sb.path))
usage(blame_usage);
-   if (lno < top || ((lno || bottom) && lno < bottom))
+   if ((!lno && (top || bottom)) || lno < bottom)
die(Q_("file %s has only %lu line",
   "file %s has only %lu lines",
   lno), path, lno);
if (bottom < 1)
bottom = 1;
-   if (top < 1)
+   if (top < 1 || lno < top)
top = lno;
bottom--;
range_set_append_unsafe(, bottom, top);
diff --git a/line-range.c b/line-range.c
index 323399d16..232c3909e 100644
--- a/line-range.c
+++ b/line-range.c
@@ -47,7 +47,7 @@ static const char *parse_loc(const char *spec, nth_line_fn_t 
nth_line,
else if (!num)
*ret = begin;
else
-   *ret = begin + num;
+   *ret = begin + num > 0 ? begin + num : 1;
return term;
}
return spec;
diff --git a/t/t8003-blame-corner-cases.sh b/t/t8003-blame-corner-cases.sh
index 661f9d430..c92a47b6d 100755
--- a/t/t8003-blame-corner-cases.sh
+++ b/t/t8003-blame-corner-cases.sh
@@ -216,14 +216,18 @@ test_expect_success 'blame -L with invalid start' '
 '
 
 test_expect_success 'blame -L with invalid end' '
-   test_must_fail git blame -L1,5 tres 2>errors &&
-   test_i18ngrep "has only 2 lines" errors
+   git blame -L1,5 tres >out &&
+   test_line_count = 2 out
 '
 
 test_expect_success 'blame parses  part of -L' '
git blame -L1,1 tres >out &&
-   cat out &&
-   test $(wc -l < out) -eq 1
+   test_line_count = 1 out
+'
+
+test_expect_success 'blame -Ln,-(n+1)' '
+   git blame -L3,-4 nine_lines >out &&
+   test_line_count = 3 out
 '
 
 test_expect_success 'indent of line numbers, nine lines' '
-- 
2.14.3 (Apple Git-98)

[PATCH] log: prevent error if line range ends past end of file

[istephens]

From: Isabella Stephens 

If the -L option is used to specify a line range in git log, and the end
of the range is past the end of the file, git will fail with a fatal
error. This commit prevents such behaviour - instead we perform the log
for existing lines within the specified range.

This commit also fixes a corner case where -L ,-n:file would be treated
as a log over the whole file. Now we treat this as -L 1,-n:file and
blame the first line of the file instead.

Signed-off-by: Isabella Stephens 
---
 line-log.c  | 8 +---
 t/t4211-line-log.sh | 8 +---
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/line-log.c b/line-log.c
index cdc2257db..4da40dec1 100644
--- a/line-log.c
+++ b/line-log.c
@@ -599,11 +599,13 @@ parse_lines(struct commit *commit, const char *prefix, 
struct string_list *args)
lines, anchor, , ,
full_name))
die("malformed -L argument '%s'", range_part);
-   if (lines < end || ((lines || begin) && lines < begin))
-   die("file %s has only %lu lines", name_part, lines);
+   if ((!lines && (begin || end)) || lines < begin)
+   die(Q_("file %s has only %lu line",
+  "file %s has only %lu lines",
+  lines), name_part, lines);
if (begin < 1)
begin = 1;
-   if (end < 1)
+   if (end < 1 || lines < end)
end = lines;
begin--;
line_log_data_insert(, full_name, begin, end);
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index d0377fae5..c617347c4 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -60,7 +60,6 @@ test_bad_opts "-L 1:nonexistent" "There is no path"
 test_bad_opts "-L 1:simple" "There is no path"
 test_bad_opts "-L '/foo:b.c'" "argument not .start,end:file"
 test_bad_opts "-L 1000:b.c" "has only.*lines"
-test_bad_opts "-L 1,1000:b.c" "has only.*lines"
 test_bad_opts "-L :b.c" "argument not .start,end:file"
 test_bad_opts "-L :foo:b.c" "no match"
 
@@ -86,12 +85,7 @@ test_expect_success '-L ,Y (Y == nlines)' '
 
 test_expect_success '-L ,Y (Y == nlines + 1)' '
n=$(expr $(wc -l 

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Martin Ågren]

On 28 May 2018 at 23:45, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
 +error:   sub/added
 +error:   sub/addedtoo
 +error: Please move or remove them before you switch branches.
  Aborting
  EOF
>>>
>>> This shows the typical effect of this series, which (I subjectively
>>> think) gives us a more pleasant end-user experience.
>>
>> Also, very subjectively, I'm torn about this. To me, just one
>> "error/warning/fatal" at the start of the first paragraph feels much
>> better. If we have to somehow mark the second paragraph that "this is
>> also part of the error message" then it's probably better to rephrase.

Would you feel the same about "hint: "? We already do prefix all the
lines there. It seems to we we should probably do the same for "hint: "
as for "warning: ", whatever we decide is right.

> I personally can go either way.  If you prefer less noisy route, we
> could change the function signature of vreportf() to take a prefix
> for the first line and another prefix for the remaining lines and
> pass that through down to the "split and print with prefix" helper.
>
> That way, we can
>
>  - allow callers to align 1st prefix (e.g. "error: ") with the
>leading indent for the second and subsequent lines by passing the
>second prefix with appropriate display width.

I suspect this second prefix would always consist of
strlen(first_prefix) spaces? We should be able to construct it on the
fly, without any need for manual counting and human mistakes.

>
>  - allow translators to grow or shrink number of lines a given
>message takes, and to decide where in the translated string to
>wrap lines.
>
> Even though step 3/3 may become a bit awkward (the second prefix
> would most likely be only whitespace, and you'd need to write
> something silly like _("\t")), we can still keep the alignment if we
> wanted to.

Thanks both for your comments. I'll see what I can come up with.

Martin

Re: [PATCH v3 00/20] Integrate commit-graph into 'fsck' and 'gc'

[Junio C Hamano]

Derrick Stolee  writes:

> Thanks for all the feedback on v2. I've tried to make this round's
> review a bit easier by splitting up the commits into smaller pieces.
> Also, the test script now has less boilerplate and uses variables and
> clear arithmetic to explain which bytes are being modified.
>
> One other change worth mentioning: in "commit-graph: add '--reachable'
> option" I put the ref-iteration into a new external
> 'write_commit_graph_reachable()' method inside commit-graph.c. This
> makes the 'gc: automatically write commit-graph files' a simpler change.

I finally managed to find time to resolve conflicts this topic has
with other topics (of your own included, if I am not mistaken).
Please double check the resolution when I push out the day's
integration result later today.

Thanks.

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Duy Nguyen  writes:

>>> +error:   sub/added
>>> +error:   sub/addedtoo
>>> +error: Please move or remove them before you switch branches.
>>>  Aborting
>>>  EOF
>>
>> This shows the typical effect of this series, which (I subjectively
>> think) gives us a more pleasant end-user experience.
>
> Also, very subjectively, I'm torn about this. To me, just one
> "error/warning/fatal" at the start of the first paragraph feels much
> better. If we have to somehow mark the second paragraph that "this is
> also part of the error message" then it's probably better to rephrase.

I personally can go either way.  If you prefer less noisy route, we
could change the function signature of vreportf() to take a prefix
for the first line and another prefix for the remaining lines and
pass that through down to the "split and print with prefix" helper.

That way, we can

 - allow callers to align 1st prefix (e.g. "error: ") with the
   leading indent for the second and subsequent lines by passing the
   second prefix with appropriate display width.

 - allow translators to grow or shrink number of lines a given
   message takes, and to decide where in the translated string to
   wrap lines.

Even though step 3/3 may become a bit awkward (the second prefix
would most likely be only whitespace, and you'd need to write
something silly like _("\t")), we can still keep the alignment if we
wanted to.

Re: [RFC PATCH v4] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Tiago Botelho  writes:

>> Running this test number of times gives me spurious errors.  Is the
>> order of these output lines unstable?  How do we "sort" these
>> bisect-all results?  If we are not sorting and the output depends on
>> happenstance, then probably we would need to compare the expected
>> and actual output after sorting.  Or if the output depends on
>> something under our control (e.g. they are related to topology and
>> relative commit timestamp), we probably should try to control that
>> "something" tighter so that we can rely on the order of the lines in
>> the "expect" file.
>
> The reason why the tests were failing was because the above "old" tests
> did not make use of test_commit which in turn would make the sha of each
> commit be different and as a result give unexpected outputs at times.
> If I move them to the top of that file the tests will pass every time,
> would that
> be ok?

That means if somebody else needs to add other tests before the
location you are planning to move your tests, your tests would
suddenly start failing, no?  That does not sound like a particularly
robust solution to me.

Proposal

[Miss Zeliha Omer Faruk]

--
Hello

I have been trying to contact you. Did you get my business proposal?

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turke

Re: Weird revision walk behaviour

[SZEDER Gábor]

> On 24/05/2018 23:26, Kevin Bracey wrote:
> >
> >>> On Wed, May 23, 2018 at 07:10:58PM +0200, SZEDER Gábor wrote:
> >>>
>     $ git log --oneline master..ba95710a3b -- ci/
>     ea44c0a594 Merge branch 'bw/protocol-v2' into 
>  jt/partial-clone-proto-v2
> 
> > In this case, we're hitting a merge commit which is not on master, but 
> > it has two parents which both are.

Indeed, I didn't notice this important detail amidst the complexity of
the git history.  Thanks, with this info I could come up with a small
test case to demonstrate the issue, see below.

> Which, IIRC, means the merge commit 
> > is INTERESTING with two UNINTERESTING parents; and we are TREESAME to 
> > only one of them.
> >
> > The commit changing the logic of TREESAME you identified believes that 
> > those TREESAME changes for merges which were intended to improve 
> > fuller history modes shouldn't affect the simple history "because 
> > partially TREESAME merges are turned into normal commits". Clearly 
> > that didn't happen here.
> >
> Haven't currently got a development environment set up here, but I've 
> been looking at the code.Here's a proposal, untested, as a potential 
> starting point if anyone wants to consider a proper patch.
> 
> The simplify_history first-scan logic never actually turned merges into 
> simple commits unless they were TREESAME to a relevant/interesting 
> parent.  Anything where the TREESAME parent was UNINTERESTING was 
> retained as a merge, but had its TREESAME flag set, and that permitted 
> later simplification.
> 
> With the redefinition of the TREESAME flag, this merge commit is no 
> longer TREESAME, and as the decoration logic to refine TREESAME isn't 
> active for simplify_history, it doesn't get cleaned up (even if it would 
> be in full history?)
> 
> I think the answer may be to add an extra post-process step on the 
> initial loop to handle this special case. Something like:
> 
>          case REV_TREE_SAME:
>              if (!revs->simplify_history || !relevant_commit(p)) {
>                  /* Even if a merge with an uninteresting
>                   * side branch brought the entire change
>                   * we are interested in, we do not want
>                   * to lose the other branches of this
>                   * merge, so we just keep going.
>                   */
>                  if (ts)
>                      ts->treesame[nth_parent] = 1;
> +   /* But we note it for potential later simplification */
> +       if (!treesame_parent)
> +    treesame_parent = p;
>      continue;
>   }
> 
> ...
> 
> After loop:
> 
> + if (relevant_parents == 0 && revs->simplify_history && 
> treesame_parent) {
> +   treesame_parent->next = NULL;// Repeats code from loop - 
> share somehow?
> +   commit->parents = treesame_parent;
> +   commit->object.flags |= TREESAME;
> +   return;
> +    }
> 
>   /*
>    * TREESAME is straightforward for single-parent commits. For merge

So, without investing nearly enough time to understand what is going
on, I massaged the above diffs into this:

  ---  >8 ---

diff --git a/revision.c b/revision.c
index 4e0e193e57..0ddd2c1e8a 100644
--- a/revision.c
+++ b/revision.c
@@ -605,7 +605,7 @@ static inline int limiting_can_increase_treesame(const 
struct rev_info *revs)
 
 static void try_to_simplify_commit(struct rev_info *revs, struct commit 
*commit)
 {
-   struct commit_list **pp, *parent;
+   struct commit_list **pp, *parent, *treesame_parents = NULL;
struct treesame_state *ts = NULL;
int relevant_change = 0, irrelevant_change = 0;
int relevant_parents, nth_parent;
@@ -672,6 +672,7 @@ static void try_to_simplify_commit(struct rev_info *revs, 
struct commit *commit)
switch (rev_compare_tree(revs, p, commit)) {
case REV_TREE_SAME:
if (!revs->simplify_history || !relevant_commit(p)) {
+   struct commit_list *tp;
/* Even if a merge with an uninteresting
 * side branch brought the entire change
 * we are interested in, we do not want
@@ -680,6 +681,13 @@ static void try_to_simplify_commit(struct rev_info *revs, 
struct commit *commit)
 */
if (ts)
ts->treesame[nth_parent] = 1;
+   /* But we note it for potential later
+* simplification
+*/
+   tp = treesame_parents;
+   treesame_parents = 
xmalloc(sizeof(*treesame_parents));
+   treesame_parents->item = p;
+   treesame_parents->next = tp;
continue;
  

RFC: Merge-related plans

[Elijah Newren]

Hi everyone,

I have some merge-related plans (and work in progress) that I'd like
to get some feedback on in order to find what order would be best to
address things in, if there are special steps I should take while
approaching some of the bigger items, and even if folks disagree with
any of the plans.


Currently, I would like to:

A) Fix cases where directory rename detection does not work with
   rebase/am due to how they call merge-recursive.

   Notes: Could just wait for D & E to land before fixing.
   Alternatively, email RFC to list now explaining issues and how the
   fix has performance implications; poll for opinions on whether to
   fix before or after D.

B) Implement a remerge-diff ability (compare a merge commit to what an
   "automatic merge" would look like)[1].

   Notes: Possibly for cherry-picks and reverts too.  Depends on C &
   E.

C) Modify/extend how path-based and mode-based conflicts are
   communicated to the user.

   Notes: Particularly important as a mechanism for handling
   challenges[2] with implementing the remerge-diff ability.  Need to
   send RFC to list with ideas to get feedback.

D) Improve merge performance.

   Notes: Includes 4-5 specific optimizations[5], some of which I
   expect to be somewhat invasive and thus may make more sense to just
   make part of the new merge strategy implemented in E.  Biggest
   optimization depends on F.

E) Write a new merge strategy meant to replace merge-recursive.

   Notes: Suggested by Junio[3][4].  Depends on F & G.

F) Make file collision conflict types more consistent.

   Notes: Specifically, make rename/rename(2to1) & rename/add
   conflicts behave more like add/add[6][7].  Depends on part of G.
   Would prefer H to be accepted first.

G) Improve merge-related portion of testsuite.

   Notes: Intended to help test new merge strategy with more
   confidence.  Will include approximately a dozen edge and corner
   cases where merge-recursive currently falls short.  Started at [8];
   see also [9].

H) Miscellaneous code cleanups irritating me while working on other
   changes[10].


My current plan was to work roughly in reverse, from H to A.  Some questions:

  * Does any of this look objectionable?
  * Should I post RFC questions on A and C earlier?
  * Should I split D and G?  (Current plan was to keep D together, but
split G into five short slightly inter-dependent topics)
  * E is kind of big.  Are there any specific things folks would like to see
with how that is handled?


Thanks,
Elijah


[1] https://bugs.chromium.org/p/git/issues/detail?id=12 [remerge-diff]
[2] 
https://public-inbox.org/git/CABPp-BEsTOZ-tCvG1y5a0qPB8xJLLa0obyTU===mbgxc1jx...@mail.gmail.com/
[remerge-diff challenges]
[3] https://public-inbox.org/git/xmqqd147kpdm@gitster.mtv.corp.google.com/
[Ideal world merge strategy]
[4] https://public-inbox.org/git/xmqqk1ydkbx0@gitster.mtv.corp.google.com/
[New strategy]
[5] Some of which are included in
https://public-inbox.org/git/20171120221944.15431-1-new...@gmail.com/
[perf improvements]
[6] 
https://public-inbox.org/git/capc5davu8vv9rdgon8jixeo3ycdvqq38yszzc-cpo+aqcak...@mail.gmail.com/
[discussion of add/add conflict resolution]
[7] https://public-inbox.org/git/20180305171125.22331-1-new...@gmail.com/
[file collision consistency RFC series]
[8] https://public-inbox.org/git/20180524070439.6367-1-new...@gmail.com/
[testcase cleanup]
[9] 
https://public-inbox.org/git/CABPp-BFc1OLYKzS5rauOehvEugPc0oGMJp-NMEAmVMW7QR=4...@mail.gmail.com/
[weird corner cases]
[10] https://public-inbox.org/git/20180522004327.13085-1-new...@gmail.com/
 [code cleanups]

Proposal

[Miss Zeliha Omer Faruk]

--
Hello

I have been trying to contact you. Did you get my business proposal?

Best Regards,
Miss.Zeliha ömer faruk
Esentepe Mahallesi Büyükdere
Caddesi Kristal Kule Binasi
No:215
Sisli - Istanbul, Turke

Re: does a new repo actually *need* default template content?

[Ævar Arnfjörð Bjarmason]

On Mon, May 28 2018, Robert P. J. Day wrote:

>   (apologies for more pedantic nitpickery, just little things i'm
> running across in my travels. aside: i actually teach git courses, so
> it's a bit embarrassing that i don't know some of this stuff. *sigh*.)

Aside from maybe the empty branches/ directory (see c8a58ac5a5 ("Revert
"Don't create the $GIT_DIR/branches directory on init"", 2009-10-31)),
none of this is needed.

I wish we didn't create any of this stuff, but have never been inclined
to make that my hill to die on.

I think we're much better off just shipping e.g. a single README file in
hooks/, or just nothing at all.

We should at least do something different on "git init --bare than" a
plain "git clone". By a quick check I have 20MB worth of 4 thousand
copies of 36 unique variations .git/hooks versions in my local area
where I clone random stuff to, none of which uses any hooks.

I think we shouldn't do anything on --bare either, except maybe a small
README_GIT_REPOSITORY_FORMAT which would refer to
gitrepository-layout(5) and a system-wide template directory.

This might have been more useful back in the early days at a time where
it was common for git users to host their own repositories, but almost
nobody does that anymore, and if they do they're expert level users who
can just get these hooks with a "cp -R".



>   running on fully-updated fedora 28 system:
>
>   $ git --version
>   git version 2.17.0
>   $
>
>   is there anything in /usr/share/git-core/templates/ that is actually
> *essential* when initializing a new repo? this is what's in my
> directory by that name:
>
>   ├── branches
>   ├── description
>   ├── hooks
>   │ ├── applypatch-msg.sample
>   │ ├── commit-msg.sample
>   │ ├── fsmonitor-watchman.sample
>   │ ├── post-update.sample
>   │ ├── pre-applypatch.sample
>   │ ├── pre-commit.sample
>   │ ├── prepare-commit-msg.sample
>   │ ├── pre-push.sample
>   │ ├── pre-rebase.sample
>   │ ├── pre-receive.sample
>   │ └── update.sample
>   └── info
>   └── exclude
>
> but none of that above looks critically important.
>
>   "man gitrepository-layout" describes the "branches" directory as
> "slightly deprecated", the default description file has a generic
> "Unnamed repository" message but, hey, so does the git source code
> repo itself, the hooks are all "commented out", and the info/exclude
> file effectively has no content, so i'm guessing that nothing there
> actually needs to be used to populate a new repo via "git init",
> correct?
>
>   under the circumstances, then, should it be a viable option to
> initialize a new repo while specifying you want *no* initial template
> content? it appears you can do that just by specifying a bogus
> template directory (or even /dev/null) with "--template=", but that
> generates a "warning" -- does a selection like that even merit a
> "warning" if it's clear that's what i'm trying to do?
>
> rday

Re: [PATCH 1/2] make show-index a builtin

[Jeff King]

On Tue, May 29, 2018 at 12:31:53AM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > diff --git a/show-index.c b/builtin/show-index.c
> > similarity index 96%
> > rename from show-index.c
> > rename to builtin/show-index.c
> > index 1ead41e211..65fa86dd08 100644
> > --- a/show-index.c
> > +++ b/builtin/show-index.c
> > @@ -4,7 +4,7 @@
> 
> I squashed
> 
> #include "builtin.h"
> 
> near the beginning of this file to squelch -DDEVELOPER extra
> warnings.  Otherwise looks obviously good.

Thanks. I'm still using my custom build options, and obviously I need to
add -Wmissing-prototypes.

> > diff --git a/git.c b/git.c
> > index 5771d62a32..c91e144d9a 100644
> > --- a/git.c
> > +++ b/git.c
> > @@ -470,6 +470,7 @@ static struct cmd_struct commands[] = {
> > { "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
> > { "show", cmd_show, RUN_SETUP },
> > { "show-branch", cmd_show_branch, RUN_SETUP },
> > +   { "show-index", cmd_show_index },
> 
> Hmph, this does not need SETUP?  Ah, of course, because its
> subcommand body used to do everything itself, and this patch just
> turns cmd_main() to cmd_show_index().

It is not even that it does the setup itself. It does not need the setup
at all, because it is purely a mechanical parsing of the stdin stream
to text output. No repo required, and it does not know or care if the
matching packfile even exists.

-Peff

Re: [PATCH] upload-pack: reject shallow requests that would return nothing

[Duy Nguyen]

On Mon, May 28, 2018 at 7:55 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> To avoid this, if rev-list returns nothing, we abort the clone/fetch.
>> The user could adjust their request (e.g. --shallow-since further back
>> in the past) and retry.
>
> Yeah, that makes sense.
>
>> Another possible option for this case is to fall back to a default
>> depth (like depth 1). But I don't like too much magic that way because
>> we may return something unexpected to the user.
>
> I agree that it would be a horrible fallback.  I actually am
> wondering if we should just silently return no objects without even
> telling the user there is something unexpected happening.  After
> all, the user may well be expecting with --shallow-since that is too
> recent that the fetch may not result in pulling anything new, and
> giving a "die" message, which now needs to be distinguished from
> other forms of die's like network connectivity or auth failures, is
> not all that helpful.

An empty fetch is probably ok (though I would need to double check if
anything bad would happen or git-fetch would give some helpful
suggestion). git-clone on the other hand should actually clean this up
with a good advice. I'll need to check and come back with v2 later.
-- 
Duy

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Duy Nguyen]

On Fri, May 25, 2018 at 11:00 PM, Martin Ågren  wrote:
> advice.c contains a useful code snippet which takes a multi-line string
> and prints the lines, prefixing and suffixing each line with two
> constant strings. This was originally added in 23cb5bf3b3 (i18n of
> multi-line advice messages, 2011-12-22) to produce such output:
>
> hint: some multi-line advice
> hint: prefixed with "hint: "
>
> The prefix is actually colored after 960786e761 (push: colorize errors,
> 2018-04-21) and each line has a suffix for resetting the color.
>
> The next commit will teach the same "prefix all the lines"-trick to the
> code that produces, e.g., "warning: "-messages. In preparation for that,
> extract the code for printing the individual lines and expose it through
> git-compat-util.h.
>
> Signed-off-by: Martin Ågren 
> ---
> I'm open for suggestions on the naming of `prefix_suffix_lines()`...

I think the important verb, print (to FILE*), is somehow missing. This
current name would be great if it produces another str(buf).
-- 
Duy

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Duy Nguyen]

On Mon, May 28, 2018 at 11:25 AM, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> diff --git a/t/t1011-read-tree-sparse-checkout.sh 
>> b/t/t1011-read-tree-sparse-checkout.sh
>> index 0c6f48f302..31b0702e6c 100755
>> --- a/t/t1011-read-tree-sparse-checkout.sh
>> +++ b/t/t1011-read-tree-sparse-checkout.sh
>> @@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
>> worktree' '
>>   test_must_fail git checkout top 2>actual &&
>>   cat >expected <<\EOF &&
>>  error: The following untracked working tree files would be overwritten by 
>> checkout:
>> - sub/added
>> - sub/addedtoo
>> -Please move or remove them before you switch branches.
>> +error:   sub/added
>> +error:   sub/addedtoo
>> +error: Please move or remove them before you switch branches.
>>  Aborting
>>  EOF
>
> This shows the typical effect of this series, which (I subjectively
> think) gives us a more pleasant end-user experience.

Also, very subjectively, I'm torn about this. To me, just one
"error/warning/fatal" at the start of the first paragraph feels much
better. If we have to somehow mark the second paragraph that "this is
also part of the error message" then it's probably better to rephrase.
-- 
Duy

Re: [PATCH v3 08/20] commit-graph: verify required chunks are present

[Jakub Narebski]

Derrick Stolee  writes:

> The commit-graph file requires the following three chunks:
>
> * OID Fanout
> * OID Lookup
> * Commit Data
>
> If any of these are missing, then the 'verify' subcommand should
> report a failure. This includes the chunk IDs malformed or the
> chunk count is truncated.

Minor nit: it should IMVHO either be "or the chunk count truncated", or
"or when the chunk count is truncated".

>
> Signed-off-by: Derrick Stolee 
> ---
>  commit-graph.c  |  9 +
>  t/t5318-commit-graph.sh | 29 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 55b41664ee..06e3e4f9ba 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -860,5 +860,14 @@ int verify_commit_graph(struct commit_graph *g)
>   return 1;
>   }
>  
> + verify_commit_graph_error = 0;
> +

By the way, if chunk count is less than 3, then by pigeonhole principle
at least one required chunk is missing.

> + if (!g->chunk_oid_fanout)
> + graph_report("commit-graph is missing the OID Fanout chunk");
> + if (!g->chunk_oid_lookup)
> + graph_report("commit-graph is missing the OID Lookup chunk");
> + if (!g->chunk_commit_data)
> + graph_report("commit-graph is missing the Commit Data chunk");

Nice and simple.  Good.

> +
>   return verify_commit_graph_error;
>  }
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index bd64481c7a..4ef3fe3dc2 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -249,6 +249,15 @@ test_expect_success 'git commit-graph verify' '
>  
>  GRAPH_BYTE_VERSION=4
>  GRAPH_BYTE_HASH=5
> +GRAPH_BYTE_CHUNK_COUNT=6
> +GRAPH_CHUNK_LOOKUP_OFFSET=8
> +GRAPH_CHUNK_LOOKUP_WIDTH=12
> +GRAPH_CHUNK_LOOKUP_ROWS=5
> +GRAPH_BYTE_OID_FANOUT_ID=$GRAPH_CHUNK_LOOKUP_OFFSET
> +GRAPH_BYTE_OID_LOOKUP_ID=`expr $GRAPH_CHUNK_LOOKUP_OFFSET + \
> +   1 \* $GRAPH_CHUNK_LOOKUP_WIDTH`
> +GRAPH_BYTE_COMMIT_DATA_ID=`expr $GRAPH_CHUNK_LOOKUP_OFFSET + \
> + 2 \* $GRAPH_CHUNK_LOOKUP_WIDTH`
>  
>  # usage: corrupt_graph_and_verify   
>  # Manipulates the commit-graph file at the position
> @@ -283,4 +292,24 @@ test_expect_success 'detect bad hash version' '
>   "hash version"
>  '
>  
> +test_expect_success 'detect bad chunk count' '
> + corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\02" \
> + "missing the Commit Data chunk"
> +'

As I wrote before, this test assumes that the last chunk (the one not
counted because of changed / corrupted chunk count) is the Commit Data
chunk.  This may be true for corrent implementation, but it is not
required by the format.

Better solution would be to check for "missing the .* chunk"; as I
understand you can pass the regexp to grep, not only strings.


Another thing would be to check if there are gaps in the file, or if the
whole file is being used.  Changing chunk count to a smaller number
would mean that chunks would not cover the rest of files.

By the way, would the following be detected:

corrupt_graph_and_verify $GRAPH_BYTE_CHUNK_COUNT "\05"

that is corrupting the chunk count to be larger than the number of
actual chunks?  Or is it left for later?

> +
> +test_expect_success 'detect missing OID fanout chunk' '
> + corrupt_graph_and_verify $GRAPH_BYTE_OID_FANOUT_ID "\0" \

We could have used "X" or " " in place of "\0", but admittedly the
latter is a better check - it also checks if there are problems with
handling of NUL character ("\0") in chunk names.

> + "missing the OID Fanout chunk"
> +'
> +
> +test_expect_success 'detect missing OID lookup chunk' '
> + corrupt_graph_and_verify $GRAPH_BYTE_OID_LOOKUP_ID "\0" \
> + "missing the OID Lookup chunk"
> +'
> +
> +test_expect_success 'detect missing commit data chunk' '
> + corrupt_graph_and_verify $GRAPH_BYTE_COMMIT_DATA_ID "\0" \
> + "missing the Commit Data chunk"
> +'

What happens if the terminating pseudo-chunk name "\0\0\0\0" gets
corrupted?  Would it be detected (or maybe it is handled by later patch
in the series)?

> +
>  test_done

Re: [RFC PATCH v4] Implement --first-parent for git rev-list --bisect

[Tiago Botelho]

On 28 May 2018 at 15:25, Junio C Hamano  wrote:

> Tiago Botelho  writes:

> > This will enable users to implement bisecting on first parents
> > which can be useful for when the commits from a feature branch
> > that we want to merge are not always tested.
> >
> > Signed-off-by: Tiago Botelho 
> > ---
> >
> > This patch adds all Junio's suggestions, namely do_find_bisection()
being
> > broken when assigning q's weight to p if in first parent mode and q
being
> > not UNINTERESTING and its weight still not being known.
> >
> > The graph displayed in the unit tests was also changed from being
top-bottom
> > to be left-right in order to keep it consistent with graphs in other
tests.
> >
> >  bisect.c   | 45
++---
> >  bisect.h   |  3 ++-
> >  builtin/rev-list.c |  3 +++
> >  revision.c |  3 ---
> >  t/t6002-rev-list-bisect.sh | 37 +
> >  5 files changed, 72 insertions(+), 19 deletions(-)

> > diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> > index a66140803..774d9a4fd 100755
> > --- a/t/t6002-rev-list-bisect.sh
> > +++ b/t/t6002-rev-list-bisect.sh
> > @@ -263,4 +263,41 @@ test_expect_success 'rev-parse --bisect can
default to good/bad refs' '
> > ...
> > +test_output_expect_success "--bisect-all --first-parent" 'git rev-list
--bisect-all --first-parent FX ^A' < > +$(git rev-parse EX) (dist=1)
> > +$(git rev-parse D) (dist=1)
> > +$(git rev-parse FX) (dist=0)
> > +EOF
> > +
> >  test_done

> Running this test number of times gives me spurious errors.  Is the
> order of these output lines unstable?  How do we "sort" these
> bisect-all results?  If we are not sorting and the output depends on
> happenstance, then probably we would need to compare the expected
> and actual output after sorting.  Or if the output depends on
> something under our control (e.g. they are related to topology and
> relative commit timestamp), we probably should try to control that
> "something" tighter so that we can rely on the order of the lines in
> the "expect" file.

The reason why the tests were failing was because the above "old" tests
did not make use of test_commit which in turn would make the sha of each
commit be different and as a result give unexpected outputs at times.
If I move them to the top of that file the tests will pass every time,
would that
be ok?

> It also appears that we have "--bisect and --first-parent do not
> work well together" in t6000, which also needs to be updated.  I
> needed the following squashed into this patch to make "make test"
> pass.

> diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
> index 969e4e9e52..981198ae6e 100755
> --- a/t/t6000-rev-list-misc.sh
> +++ b/t/t6000-rev-list-misc.sh
> @@ -96,8 +96,8 @@ test_expect_success 'rev-list can show index objects' '
>  test_cmp expect actual
>   '

> -test_expect_success '--bisect and --first-parent can not be combined' '
> -   test_must_fail git rev-list --bisect --first-parent HEAD
> +test_expect_success '--bisect and --first-parent can now be combined' '
> +   git rev-list --bisect --first-parent HEAD
>   '

>   test_expect_success '--header shows a NUL after each commit' '

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Ævar Arnfjörð Bjarmason]

On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>>> @@ -1554,23 +1554,42 @@ filter..smudge::
>>> +Depending on the circumstances it might be better to use
>>> +`fsck.skipList` instead to explicitly whitelist those objects that
>>> +have issues, otherwise new occurrences of the same issue will be
>
> I had to read the "instead to ..." part three times before
> convincing myself that this is a good description, and I agree with
> the assessment.  Perhaps we would want to make the recommendation a
> bit stronger, even.
>
> In general, it is better to enumerate existing objects with
> problems with skipList, instead of listing the kind of
> breakages these problematic objects share to be ignored, as
> doing the latter will allow new instances of the same
> breakages go unnoticed.
>
> If the project has some tool constraints and have to accept new
> "broken" objects on ongoing basis, then fsck. facility may
> make sense, but that is probably a very narrow special use case.

That makes sense. I'll reword this bit.

>>> +hidden going forward, although that's unlikely to happen in practice
>>> +unless someone is being deliberately malicious.
>>
>> Is it worth mentioning buggy tools and/or other buggy Git
>> implementations as sources?
>
> Or old Git implementations we ourselves shipped.  I do not think it
> is worth mentioning it, but I do agree that "unless somebody is
> being deliberatly malicious" is misleading, if that is what you are
> getting at.  One use of fsck is about noticing that you are under
> attack, so "unless someone is being malicious" there in the sentence
> does not make sense.  When somebody is attacking you, you do not
> want to use fsck. to ignore it.
>
> But that becomes a moot point, if we were to follow the line of
> rewrite I suggested above.

I'll try to clarify this, but I think we really should have some bit
there about historical tools. Realistically no new git tools produce
these, so the user needs to be made aware of what the trade-off of
turning these on is.

The reality of that is that these objects are exceedingly rare, and
mostly found in various old repositories. Something like that need to
be mentioned so the user can weigh the trade-off of turning this on.

Re: [PATCH 1/2] make show-index a builtin

[Junio C Hamano]

Jeff King  writes:

> diff --git a/show-index.c b/builtin/show-index.c
> similarity index 96%
> rename from show-index.c
> rename to builtin/show-index.c
> index 1ead41e211..65fa86dd08 100644
> --- a/show-index.c
> +++ b/builtin/show-index.c
> @@ -4,7 +4,7 @@

I squashed

#include "builtin.h"

near the beginning of this file to squelch -DDEVELOPER extra
warnings.  Otherwise looks obviously good.

>  static const char show_index_usage[] =
>  "git show-index";
>  
> -int cmd_main(int argc, const char **argv)
> +int cmd_show_index(int argc, const char **argv, const char *prefix)
>  {
>   int i;
>   unsigned nr;
> diff --git a/git.c b/git.c
> index 5771d62a32..c91e144d9a 100644
> --- a/git.c
> +++ b/git.c
> @@ -470,6 +470,7 @@ static struct cmd_struct commands[] = {
>   { "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
>   { "show", cmd_show, RUN_SETUP },
>   { "show-branch", cmd_show_branch, RUN_SETUP },
> + { "show-index", cmd_show_index },

Hmph, this does not need SETUP?  Ah, of course, because its
subcommand body used to do everything itself, and this patch just
turns cmd_main() to cmd_show_index().



>   { "show-ref", cmd_show_ref, RUN_SETUP },
>   { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
>   { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },

Re: [RFC PATCH v4] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Tiago Botelho  writes:

> This will enable users to implement bisecting on first parents
> which can be useful for when the commits from a feature branch
> that we want to merge are not always tested.
>
> Signed-off-by: Tiago Botelho 
> ---
>
> This patch adds all Junio's suggestions, namely do_find_bisection() being
> broken when assigning q's weight to p if in first parent mode and q being
> not UNINTERESTING and its weight still not being known.
>
> The graph displayed in the unit tests was also changed from being top-bottom
> to be left-right in order to keep it consistent with graphs in other tests.
>
>  bisect.c   | 45 ++---
>  bisect.h   |  3 ++-
>  builtin/rev-list.c |  3 +++
>  revision.c |  3 ---
>  t/t6002-rev-list-bisect.sh | 37 +
>  5 files changed, 72 insertions(+), 19 deletions(-)

> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> index a66140803..774d9a4fd 100755
> --- a/t/t6002-rev-list-bisect.sh
> +++ b/t/t6002-rev-list-bisect.sh
> @@ -263,4 +263,41 @@ test_expect_success 'rev-parse --bisect can default to 
> good/bad refs' '
> ...
> +test_output_expect_success "--bisect-all --first-parent" 'git rev-list 
> --bisect-all --first-parent FX ^A' < +$(git rev-parse EX) (dist=1)
> +$(git rev-parse D) (dist=1)
> +$(git rev-parse FX) (dist=0)
> +EOF
> +
>  test_done

Running this test number of times gives me spurious errors.  Is the
order of these output lines unstable?  How do we "sort" these
bisect-all results?  If we are not sorting and the output depends on
happenstance, then probably we would need to compare the expected
and actual output after sorting.  Or if the output depends on
something under our control (e.g. they are related to topology and
relative commit timestamp), we probably should try to control that
"something" tighter so that we can rely on the order of the lines in
the "expect" file.

It also appears that we have "--bisect and --first-parent do not
work well together" in t6000, which also needs to be updated.  I
needed the following squashed into this patch to make "make test"
pass.

diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 969e4e9e52..981198ae6e 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -96,8 +96,8 @@ test_expect_success 'rev-list can show index objects' '
test_cmp expect actual
 '
 
-test_expect_success '--bisect and --first-parent can not be combined' '
-   test_must_fail git rev-list --bisect --first-parent HEAD
+test_expect_success '--bisect and --first-parent can now be combined' '
+   git rev-list --bisect --first-parent HEAD
 '
 
 test_expect_success '--header shows a NUL after each commit' '

Re: why does builtin/init-db.c mention "/etc/core-git/templates/hooks/update"?

[Robert P. J. Day]

On Mon, 28 May 2018, Sitaram Chamarty wrote:

> On Mon, May 28, 2018 at 09:27:18AM -0400, Robert P. J. Day wrote:
>
> [snipped the rest because I really don't know]
>
> >   more to the point, is that actually what the "update" hook does? i
> > just looked at the shipped sample, "update.sample", and it seems to be
> > related to tags:
> >
> >   #!/bin/sh
> >   #
> >   # An example hook script to block unannotated tags from entering.
>
> no that's just a sample.  An update hook can do pretty much
> anything, and if it exits with 0 status code, the actual update
> succeeds.  If it exists with any non-zero exit code, the update will
> fail.
>
> This is (usually) the basis for a lot of checks that people may
> want, from commit message format to access control at the ref
> (branch/tag) level for write operations.

  i'm not convinced that that reference is "just a sample." the
comment in builtin/init-db.c reads:

  /* Note: if ".git/hooks" file exists in the repository being
   * re-initialized, /etc/core-git/templates/hooks/update would
   * cause "git init" to fail here...

the reference to "/etc/core-git/templates/hooks/update" might lead
some people to believe that that somehow refers to the "update.sample"
hook that comes with a new repo, but that hook is specifically related
to git-receive-pack, and has nothing to do with re-initalizing a repo,
AFAICT.

  i'm just suggesting that that comment in buildin/init-db.c seems
more than a little inaccurate.

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH v3 07/20] commit-graph: verify catches corrupt signature

[Jakub Narebski]

Derrick Stolee  writes:

> This is the first of several commits that add a test to check that
> 'git commit-graph verify' catches corruption in the commit-graph
> file. The first test checks that the command catches an error in
> the file signature. This is a check that exists in the existing
> commit-graph reading code.

Good start.

This handles 3 out of 5 checks in load_commit_graph_one().  The
remaining are:
* too short file (length smaller than minimal commit-graph size)
* more than one chunk of one of 4 defined types

> Add a helper method 'corrupt_graph_and_verify' to the test script
> t5318-commit-graph.sh. This helper corrupts the commit-graph file
> at a certain location, runs 'git commit-graph verify', and reports
> the output to the 'err' file. This data is filtered to remove the
> lines added by 'test_must_fail' when the test is run verbosely.
> Then, the output is checked to contain a specific error message.

Thanks for an explanation.

> Signed-off-by: Derrick Stolee 
> ---
>  t/t5318-commit-graph.sh | 43 +++
>  1 file changed, 43 insertions(+)
>
> diff --git a/t/t5318-commit-graph.sh b/t/t5318-commit-graph.sh
> index 6ca451dfd2..bd64481c7a 100755
> --- a/t/t5318-commit-graph.sh
> +++ b/t/t5318-commit-graph.sh
> @@ -235,9 +235,52 @@ test_expect_success 'perform fast-forward merge in full 
> repo' '
>   test_cmp expect output
>  '
>  
> +# the verify tests below expect the commit-graph to contain
> +# exactly the commits reachable from the commits/8 branch.
> +# If the file changes the set of commits in the list, then the
> +# offsets into the binary file will result in different edits
> +# and the tests will likely break.
> +
>  test_expect_success 'git commit-graph verify' '
>   cd "$TRASH_DIRECTORY/full" &&
> + git rev-parse commits/8 | git commit-graph write --stdin-commits &&
>   git commit-graph verify >output
>  '

I don't quite understand what the change is meant to do.

Also, as I said earlier, I'd prefer if tests were as indpendent of each
other as possible, to make running individual tests (e.g. only
previously falling tests) easier.

I especially do not like mixing running actual test with setting up the
repository for future tests, as here.

>  
> +GRAPH_BYTE_VERSION=4
> +GRAPH_BYTE_HASH=5
> +
> +# usage: corrupt_graph_and_verify   
> +# Manipulates the commit-graph file at the position
> +# by inserting the data, then runs 'git commit-graph verify'
> +# and places the output in the file 'err'. Test 'err' for
> +# the given string.

Very nice to have this description.

> +corrupt_graph_and_verify() {
> + pos=$1
> + data="${2:-\0}"
> + grepstr=$3
> + cd "$TRASH_DIRECTORY/full" &&
> + test_when_finished mv commit-graph-backup $objdir/info/commit-graph &&
> + cp $objdir/info/commit-graph commit-graph-backup &&
> + printf "$data" | dd of="$objdir/info/commit-graph" bs=1 seek="$pos" 
> conv=notrunc &&

Using 'printf' with octal is much more portable than relying on 'echo'
supporting octal escape sequences (or supporting escape sequences at
all).

> + test_must_fail git commit-graph verify 2>test_err &&
> + grep -v "^+" test_err >err
> + grep "$grepstr" err

Shouldn't this last 'grep' be 'test_i18ngrep' instead, to allow for
translated messages from 'git commit-graph verify' / 'git fsck'?

> +}

This function makes actual tests short and simple, without duplicated
code.  Very good.

> +
> +test_expect_success 'detect bad signature' '
> + corrupt_graph_and_verify 0 "\0" \
> + "graph signature"
> +'
> +
> +test_expect_success 'detect bad version' '
> + corrupt_graph_and_verify $GRAPH_BYTE_VERSION "\02" \
> + "graph version"
> +'
> +
> +test_expect_success 'detect bad hash version' '
> + corrupt_graph_and_verify $GRAPH_BYTE_HASH "\02" \

When we move from SHA-1 (hash version 1) to NewHash (hash version 2),
this test would start failing... which is actually not a bad idea.

> + "hash version"
> +'
> +
>  test_done

Re: why does builtin/init-db.c mention "/etc/core-git/templates/hooks/update"?

[Sitaram Chamarty]

On Mon, May 28, 2018 at 09:27:18AM -0400, Robert P. J. Day wrote:

[snipped the rest because I really don't know]

>   more to the point, is that actually what the "update" hook does? i
> just looked at the shipped sample, "update.sample", and it seems to be
> related to tags:
> 
>   #!/bin/sh
>   #
>   # An example hook script to block unannotated tags from entering.

no that's just a sample.  An update hook can do pretty much
anything, and if it exits with 0 status code, the actual update
succeeds.  If it exists with any non-zero exit code, the update
will fail.

This is (usually) the basis for a lot of checks that people may
want, from commit message format to access control at the ref
(branch/tag) level for write operations.

Re: protocol for updating .po gettext files?

[Junio C Hamano]

"Robert P. J. Day"  writes:

>i was going to submit a minor patch to fix grammar here:
>
> builtin/init-db.c: warning(_("templates not found %s"), template_dir);
>
> as it should display "templates not found in %s" to be consistent with
> other messages, but i know from nothing about .po files, so does one
> also have to update those, or how does that work?

You as a developer generally don't.  Instead, you just update the
code i.e.

-   warning(_("t not found %s"), t);
+   warning(_("t not found in %s"), t);

and leave it up to the i18n coordinator to update *.pot and *.po so
that localization teams can work on it, which all happens closer to
the next release.

I think po/README has a bit more details describing which part is
handled by whom.

why does builtin/init-db.c mention "/etc/core-git/templates/hooks/update"?

[Robert P. J. Day]

  just noticed this in builtin/init-db.c:

... snip ...
#ifndef DEFAULT_GIT_TEMPLATE_DIR
#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
#endif
... snip ...
static void copy_templates_1(struct strbuf *path, struct strbuf *template_path,
 DIR *dir)
{
size_t path_baselen = path->len;
size_t template_baselen = template_path->len;
struct dirent *de;

/* Note: if ".git/hooks" file exists in the repository being
 * re-initialized, /etc/core-git/templates/hooks/update would
 * cause "git init" to fail here.  I think this is sane but
 * it means that the set of templates we ship by default, along
 * with the way the namespace under .git/ is organized, should
 * be really carefully chosen.
 */
... snip ...

  should the reference to /etc/core-git/templates/hooks/update instead
refer to the directory /usr/share/git-core/templates/..., given the
default directory defined just a few lines above it? (there is no such
directory, /etc/core-git/, on my system.)

  more to the point, is that actually what the "update" hook does? i
just looked at the shipped sample, "update.sample", and it seems to be
related to tags:

  #!/bin/sh
  #
  # An example hook script to block unannotated tags from entering.
  # Called by "git receive-pack" with arguments: refname sha1-old sha1-new
  #
  # To enable this hook, rename this file to "update".

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [RFC PATCH v4] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Tiago Botelho  writes:

> diff --git a/bisect.c b/bisect.c
> index 4eafc8262..e58cb8d62 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -33,6 +33,8 @@ static const char *term_good;
>   *
>   * We care just barely enough to avoid recursing for
>   * non-merge entries.
> + *
> + * Note: This function does not support the usage --first-parent.
>   */

Hmph, is this because we know --first-parent codepath currently does
not call this function, so we do not bother to prepare this function
to be called from --first-parent codepath?

I am not saying that we must prepare this function to be callable
with --first-parent; if I have to wonder why the above comment is
there and what it is trying to say, I suspect most other readers
would, too, so...

> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> index a66140803..774d9a4fd 100755
> --- a/t/t6002-rev-list-bisect.sh
> +++ b/t/t6002-rev-list-bisect.sh
> @@ -263,4 +263,41 @@ test_expect_success 'rev-parse --bisect can default to 
> good/bad refs' '
>   test_cmp expect.sorted actual.sorted
>  '
>  
> +# We generate the following commit graph:
> +#
> +#   B - C
> +#  /  \
> +# AFX
> +#  \  /
> +#   D - EX
> +
> +test_expect_success 'setup' '
> +  test_commit A &&
> +  test_commit B &&
> +  test_commit C &&
> +  git reset --hard A &&
> +  test_commit D &&
> +  test_commit EX &&
> +  test_merge FX C
> +'
> +
> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect 
> --first-parent FX ^A' < +$(git rev-parse EX)
> +EOF
> +
> +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list 
> --bisect-vars --first-parent FX ^A' < +bisect_rev='$(git rev-parse EX)'
> +bisect_nr=1
> +bisect_good=0
> +bisect_bad=1
> +bisect_all=3
> +bisect_steps=1
> +EOF
> +
> +test_output_expect_success "--bisect-all --first-parent" 'git rev-list 
> --bisect-all --first-parent FX ^A' < +$(git rev-parse EX) (dist=1)
> +$(git rev-parse D) (dist=1)
> +$(git rev-parse FX) (dist=0)
> +EOF
> +

These are all good basic tests, but can you come up with a test that
demonstrates breakage in the previous round that has been fixed in
this version of the patch?

Thanks.

Re: [PATCH] git-rebase--interactive: fix copy-paste mistake

[Orgad Shaneh]

On Mon, May 28, 2018 at 3:56 PM Johannes Schindelin <
johannes.schinde...@gmx.de> wrote:

> Hi Orgad,

> On Sun, 27 May 2018, Orgad Shaneh wrote:

> > exec argument is a command, not a commit.
> >
> > Signed-off-by: Orgad Shaneh 
> > ---
> >   git-rebase--interactive.sh | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index cbf44f8648..85a72b933e 100644
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -160,7 +160,7 @@ r, reword  = use commit, but edit the commit
> > message
> >   e, edit  = use commit, but stop for amending
> >   s, squash  = use commit, but meld into previous commit
> >   f, fixup  = like \"squash\", but discard this commit's log
message
> > -x, exec  = run command (the rest of the line) using shell
> > +x, exec  = run command (the rest of the line) using shell

> Apart from the white-space (which I *think* might make `git apply` barf on
> this diff), this looks obviously correct to me.

I'm behind a firewall, so I just pasted the patch in gmail, hoping it will
pass correctly :)

> To make it easier for Junio, maybe you can provide a link to a branch in a
> public repository, ready to be fetched?

Pushed to https://github.com/orgads/git -> exec-description branch.

> Thank you for cleaning up my mess,

No prob :)

- Orgad

does a new repo actually *need* default template content?

[Robert P. J. Day]

  (apologies for more pedantic nitpickery, just little things i'm
running across in my travels. aside: i actually teach git courses, so
it's a bit embarrassing that i don't know some of this stuff. *sigh*.)

  running on fully-updated fedora 28 system:

  $ git --version
  git version 2.17.0
  $

  is there anything in /usr/share/git-core/templates/ that is actually
*essential* when initializing a new repo? this is what's in my
directory by that name:

  ├── branches
  ├── description
  ├── hooks
  │   ├── applypatch-msg.sample
  │   ├── commit-msg.sample
  │   ├── fsmonitor-watchman.sample
  │   ├── post-update.sample
  │   ├── pre-applypatch.sample
  │   ├── pre-commit.sample
  │   ├── prepare-commit-msg.sample
  │   ├── pre-push.sample
  │   ├── pre-rebase.sample
  │   ├── pre-receive.sample
  │   └── update.sample
  └── info
  └── exclude

but none of that above looks critically important.

  "man gitrepository-layout" describes the "branches" directory as
"slightly deprecated", the default description file has a generic
"Unnamed repository" message but, hey, so does the git source code
repo itself, the hooks are all "commented out", and the info/exclude
file effectively has no content, so i'm guessing that nothing there
actually needs to be used to populate a new repo via "git init",
correct?

  under the circumstances, then, should it be a viable option to
initialize a new repo while specifying you want *no* initial template
content? it appears you can do that just by specifying a bogus
template directory (or even /dev/null) with "--template=", but that
generates a "warning" -- does a selection like that even merit a
"warning" if it's clear that's what i'm trying to do?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: [PATCH] git-rebase--interactive: fix copy-paste mistake

[Johannes Schindelin]

Hi Orgad,

On Sun, 27 May 2018, Orgad Shaneh wrote:

> exec argument is a command, not a commit.
> 
> Signed-off-by: Orgad Shaneh 
> ---
>   git-rebase--interactive.sh | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index cbf44f8648..85a72b933e 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -160,7 +160,7 @@ r, reword  = use commit, but edit the commit
> message
>   e, edit  = use commit, but stop for amending
>   s, squash  = use commit, but meld into previous commit
>   f, fixup  = like \"squash\", but discard this commit's log message
> -x, exec  = run command (the rest of the line) using shell
> +x, exec  = run command (the rest of the line) using shell

Apart from the white-space (which I *think* might make `git apply` barf on
this diff), this looks obviously correct to me.

To make it easier for Junio, maybe you can provide a link to a branch in a
public repository, ready to be fetched?

>   d, drop  = remove commit
>   l, label  = label current HEAD with a name
>   t, reset  = reset HEAD to a label
> -- 
> 2.17.0.windows.1

Thank you for cleaning up my mess,
Dscho

[GSoC][PATCH v4 1/4] rebase: introduce a dedicated backend for --preserve-merges

[Alban Gruin]

This duplicates git-rebase--interactive.sh to
git-rebase--preserve-merges.sh. This is done to split -p from -i. No
modifications are made to this file here, but any code that is not used
by -p will be stripped in the next commit.

Signed-off-by: Alban Gruin 
---
 .gitignore |1 +
 Makefile   |2 +
 git-rebase--preserve-merges.sh | 1069 
 3 files changed, 1072 insertions(+)
 create mode 100644 git-rebase--preserve-merges.sh

diff --git a/.gitignore b/.gitignore
index 833ef3b0b..ef4925485 100644
--- a/.gitignore
+++ b/.gitignore
@@ -117,6 +117,7 @@
 /git-rebase--helper
 /git-rebase--interactive
 /git-rebase--merge
+/git-rebase--preserve-merges
 /git-receive-pack
 /git-reflog
 /git-remote
diff --git a/Makefile b/Makefile
index 50da82b01..810a0d0f4 100644
--- a/Makefile
+++ b/Makefile
@@ -582,6 +582,7 @@ SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--interactive
+SCRIPT_LIB += git-rebase--preserve-merges
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
@@ -2271,6 +2272,7 @@ LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
 LOCALIZED_SH += git-rebase--interactive.sh
+LOCALIZED_SH += git-rebase--preserve-merges.sh
 LOCALIZED_SH += git-sh-setup.sh
 LOCALIZED_PERL = $(SCRIPT_PERL)
 
diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
new file mode 100644
index 0..2f4941d0f
--- /dev/null
+++ b/git-rebase--preserve-merges.sh
@@ -0,0 +1,1069 @@
+# This shell script fragment is sourced by git-rebase to implement
+# its interactive mode.  "git rebase --interactive" makes it easy
+# to fix up commits in the middle of a series and rearrange commits.
+#
+# Copyright (c) 2006 Johannes E. Schindelin
+#
+# The original idea comes from Eric W. Biederman, in
+# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
+#
+# The file containing rebase commands, comments, and empty lines.
+# This file is created by "git rebase -i" then edited by the user.  As
+# the lines are processed, they are removed from the front of this
+# file and written to the tail of $done.
+todo="$state_dir"/git-rebase-todo
+
+# The rebase command lines that have already been processed.  A line
+# is moved here when it is first handled, before any associated user
+# actions.
+done="$state_dir"/done
+
+# The commit message that is planned to be used for any changes that
+# need to be committed following a user interaction.
+msg="$state_dir"/message
+
+# The file into which is accumulated the suggested commit message for
+# squash/fixup commands.  When the first of a series of squash/fixups
+# is seen, the file is created and the commit message from the
+# previous commit and from the first squash/fixup commit are written
+# to it.  The commit message for each subsequent squash/fixup commit
+# is appended to the file as it is processed.
+#
+# The first line of the file is of the form
+# # This is a combination of $count commits.
+# where $count is the number of commits whose messages have been
+# written to the file so far (including the initial "pick" commit).
+# Each time that a commit message is processed, this line is read and
+# updated.  It is deleted just before the combined commit is made.
+squash_msg="$state_dir"/message-squash
+
+# If the current series of squash/fixups has not yet included a squash
+# command, then this file exists and holds the commit message of the
+# original "pick" commit.  (If the series ends without a "squash"
+# command, then this can be used as the commit message of the combined
+# commit without opening the editor.)
+fixup_msg="$state_dir"/message-fixup
+
+# $rewritten is the name of a directory containing files for each
+# commit that is reachable by at least one merge base of $head and
+# $upstream. They are not necessarily rewritten, but their children
+# might be.  This ensures that commits on merged, but otherwise
+# unrelated side branches are left alone. (Think "X" in the man page's
+# example.)
+rewritten="$state_dir"/rewritten
+
+dropped="$state_dir"/dropped
+
+end="$state_dir"/end
+msgnum="$state_dir"/msgnum
+
+# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
+# GIT_AUTHOR_DATE that will be used for the commit that is currently
+# being rebased.
+author_script="$state_dir"/author-script
+
+# When an "edit" rebase command is being processed, the SHA1 of the
+# commit to be edited is recorded in this file.  When "git rebase
+# --continue" is executed, if there are any staged changes then they
+# will be amended to the HEAD commit, but only provided the HEAD
+# commit is still the commit to be edited.  When any other rebase
+# command is processed, this file is deleted.
+amend="$state_dir"/amend
+
+# For the post-rewrite hook, we make a list of rewritten 

[GSoC][PATCH v4 2/4] rebase: strip unused code in git-rebase--preserve-merges.sh

[Alban Gruin]

This removes the code coming from git-rebase--interactive.sh that is not
needed by preserve-merges, and changes the header comment accordingly.

In a following commit, the -p code from git-rebase--interactive.sh will
be stripped out. As preserve-merges’ successor is already in the works,
this will be the only script to be converted.

This also seems to fix a bug where a failure in
`pick_one_preserving_merges()` would fallback to the non-preserve-merges
`pick_one()`.

Signed-off-by: Alban Gruin 
---
 git-rebase--preserve-merges.sh | 65 +++---
 1 file changed, 4 insertions(+), 61 deletions(-)

diff --git a/git-rebase--preserve-merges.sh b/git-rebase--preserve-merges.sh
index 2f4941d0f..c51c7828e 100644
--- a/git-rebase--preserve-merges.sh
+++ b/git-rebase--preserve-merges.sh
@@ -1,12 +1,8 @@
-# This shell script fragment is sourced by git-rebase to implement
-# its interactive mode.  "git rebase --interactive" makes it easy
-# to fix up commits in the middle of a series and rearrange commits.
+# This shell script fragment is sourced by git-rebase to implement its
+# preserve-merges mode.
 #
 # Copyright (c) 2006 Johannes E. Schindelin
 #
-# The original idea comes from Eric W. Biederman, in
-# https://public-inbox.org/git/m1odwkyuf5.fsf...@ebiederm.dsl.xmission.com/
-#
 # The file containing rebase commands, comments, and empty lines.
 # This file is created by "git rebase -i" then edited by the user.  As
 # the lines are processed, they are removed from the front of this
@@ -287,17 +283,7 @@ pick_one () {
empty_args="--allow-empty"
fi
 
-   test -d "$rewritten" &&
-   pick_one_preserving_merges "$@" && return
-   output eval git cherry-pick $allow_rerere_autoupdate 
$allow_empty_message \
-   ${gpg_sign_opt:+$(git rev-parse --sq-quote 
"$gpg_sign_opt")} \
-   $signoff "$strategy_args" $empty_args $ff "$@"
-
-   # If cherry-pick dies it leaves the to-be-picked commit unrecorded. 
Reschedule
-   # previous task so this commit is not lost.
-   ret=$?
-   case "$ret" in [01]) ;; *) reschedule_last_action ;; esac
-   return $ret
+   pick_one_preserving_merges "$@"
 }
 
 pick_one_preserving_merges () {
@@ -761,11 +747,6 @@ get_missing_commit_check_level () {
 initiate_action () {
case "$1" in
continue)
-   if test ! -d "$rewritten"
-   then
-   exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
-   --continue
-   fi
# do we have anything to commit?
if git diff-index --cached --quiet HEAD --
then
@@ -824,12 +805,6 @@ first and then run 'git rebase --continue' again.")"
;;
skip)
git rerere clear
-
-   if test ! -d "$rewritten"
-   then
-   exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
-   --continue
-   fi
do_rest
return 0
;;
@@ -944,43 +919,11 @@ EOF
}
 
expand_todo_ids
-
-   test -d "$rewritten" || test -n "$force_rebase" ||
-   onto="$(git rebase--helper --skip-unnecessary-picks)" ||
-   die "Could not skip unnecessary pick commands"
-
checkout_onto
-   if test ! -d "$rewritten"
-   then
-   require_clean_work_tree "rebase"
-   exec git rebase--helper ${force_rebase:+--no-ff} 
$allow_empty_message \
-   --continue
-   fi
do_rest
 }
 
-git_rebase__interactive () {
-   initiate_action "$action"
-   ret=$?
-   if test $ret = 0; then
-   return 0
-   fi
-
-   setup_reflog_action
-   init_basic_state
-
-   init_revisions_and_shortrevisions
-
-   git rebase--helper --make-script ${keep_empty:+--keep-empty} \
-   ${rebase_merges:+--rebase-merges} \
-   ${rebase_cousins:+--rebase-cousins} \
-   $revisions ${restrict_revision+^$restrict_revision} >"$todo" ||
-   die "$(gettext "Could not generate todo list")"
-
-   complete_action
-}
-
-git_rebase__interactive__preserve_merges () {
+git_rebase__preserve_merges () {
initiate_action "$action"
ret=$?
if test $ret = 0; then
-- 
2.16.1

[GSoC][PATCH v4 3/4] rebase: use the new git-rebase--preserve-merges.sh

[Alban Gruin]

Create a new type of rebase, "preserve-merges", used when rebase is
called with -p.

Before that, the type for preserve-merges was "interactive", and some
places of this script compared $type to "interactive". Instead, the code
now checks if $interactive_rebase is empty or not, as it is set to
"explicit" when calling an interactive rebase (and, possibly, one of its
submodes), and "implied" when calling one of its
submodes (eg. preserve-merges) *without* interactive rebase.

It also detects the presence of the directory "$merge_dir"/rewritten
left by the preserve-merges script when calling rebase --continue,
--skip, etc., and, if it exists, sets the rebase mode to
preserve-merges. In this case, interactive_rebase is set to "explicit",
as "implied" would break some tests.

Signed-off-by: Alban Gruin 
---
 git-rebase.sh | 32 +---
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/git-rebase.sh b/git-rebase.sh
index 40be59ecc..19bdebb48 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -207,7 +207,14 @@ run_specific_rebase () {
autosquash=
fi
. git-rebase--$type
-   git_rebase__$type${preserve_merges:+__preserve_merges}
+
+   if test -z "$preserve_merges"
+   then
+   git_rebase__$type
+   else
+   git_rebase__preserve_merges
+   fi
+
ret=$?
if test $ret -eq 0
then
@@ -239,7 +246,12 @@ then
state_dir="$apply_dir"
 elif test -d "$merge_dir"
 then
-   if test -f "$merge_dir"/interactive
+   if test -d "$merge_dir"/rewritten
+   then
+   type=preserve-merges
+   interactive_rebase=explicit
+   preserve_merges=t
+   elif test -f "$merge_dir"/interactive
then
type=interactive
interactive_rebase=explicit
@@ -402,14 +414,14 @@ if test -n "$action"
 then
test -z "$in_progress" && die "$(gettext "No rebase in progress?")"
# Only interactive rebase uses detailed reflog messages
-   if test "$type" = interactive && test "$GIT_REFLOG_ACTION" = rebase
+   if test -n "$interactive_rebase" && test "$GIT_REFLOG_ACTION" = rebase
then
GIT_REFLOG_ACTION="rebase -i ($action)"
export GIT_REFLOG_ACTION
fi
 fi
 
-if test "$action" = "edit-todo" && test "$type" != "interactive"
+if test "$action" = "edit-todo" && test -z "$interactive_rebase"
 then
die "$(gettext "The --edit-todo action can only be used during 
interactive rebase.")"
 fi
@@ -487,7 +499,13 @@ fi
 
 if test -n "$interactive_rebase"
 then
-   type=interactive
+   if test -z "$preserve_merges"
+   then
+   type=interactive
+   else
+   type=preserve-merges
+   fi
+
state_dir="$merge_dir"
 elif test -n "$do_merge"
 then
@@ -647,7 +665,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit 
or stash them.")"
 # but this should be done only when upstream and onto are the same
 # and if this is not an interactive rebase.
 mb=$(git merge-base "$onto" "$orig_head")
-if test "$type" != interactive && test "$upstream" = "$onto" &&
+if test -z "$interactive_rebase" && test "$upstream" = "$onto" &&
test "$mb" = "$onto" && test -z "$restrict_revision" &&
# linear history?
! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > 
/dev/null
@@ -691,7 +709,7 @@ then
GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
 fi
 
-test "$type" = interactive && run_specific_rebase
+test -n "$interactive_rebase" && run_specific_rebase
 
 # Detach HEAD and reset the tree
 say "$(gettext "First, rewinding head to replay your work on top of it...")"
-- 
2.16.1

[GSoC][PATCH v4 4/4] rebase: remove -p code from git-rebase--interactive.sh

[Alban Gruin]

All the code specific to preserve-merges was moved to
git-rebase--preserve-merges.sh, and so it’s useless to keep it
here.

The intent of this commit is to clean this script as much as possible to
prepare a peaceful conversion as a builtin written in C.

Signed-off-by: Alban Gruin 
---
 git-rebase--interactive.sh | 802 +
 1 file changed, 8 insertions(+), 794 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 2f4941d0f..9884ecd71 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -13,83 +13,6 @@
 # file and written to the tail of $done.
 todo="$state_dir"/git-rebase-todo
 
-# The rebase command lines that have already been processed.  A line
-# is moved here when it is first handled, before any associated user
-# actions.
-done="$state_dir"/done
-
-# The commit message that is planned to be used for any changes that
-# need to be committed following a user interaction.
-msg="$state_dir"/message
-
-# The file into which is accumulated the suggested commit message for
-# squash/fixup commands.  When the first of a series of squash/fixups
-# is seen, the file is created and the commit message from the
-# previous commit and from the first squash/fixup commit are written
-# to it.  The commit message for each subsequent squash/fixup commit
-# is appended to the file as it is processed.
-#
-# The first line of the file is of the form
-# # This is a combination of $count commits.
-# where $count is the number of commits whose messages have been
-# written to the file so far (including the initial "pick" commit).
-# Each time that a commit message is processed, this line is read and
-# updated.  It is deleted just before the combined commit is made.
-squash_msg="$state_dir"/message-squash
-
-# If the current series of squash/fixups has not yet included a squash
-# command, then this file exists and holds the commit message of the
-# original "pick" commit.  (If the series ends without a "squash"
-# command, then this can be used as the commit message of the combined
-# commit without opening the editor.)
-fixup_msg="$state_dir"/message-fixup
-
-# $rewritten is the name of a directory containing files for each
-# commit that is reachable by at least one merge base of $head and
-# $upstream. They are not necessarily rewritten, but their children
-# might be.  This ensures that commits on merged, but otherwise
-# unrelated side branches are left alone. (Think "X" in the man page's
-# example.)
-rewritten="$state_dir"/rewritten
-
-dropped="$state_dir"/dropped
-
-end="$state_dir"/end
-msgnum="$state_dir"/msgnum
-
-# A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
-# GIT_AUTHOR_DATE that will be used for the commit that is currently
-# being rebased.
-author_script="$state_dir"/author-script
-
-# When an "edit" rebase command is being processed, the SHA1 of the
-# commit to be edited is recorded in this file.  When "git rebase
-# --continue" is executed, if there are any staged changes then they
-# will be amended to the HEAD commit, but only provided the HEAD
-# commit is still the commit to be edited.  When any other rebase
-# command is processed, this file is deleted.
-amend="$state_dir"/amend
-
-# For the post-rewrite hook, we make a list of rewritten commits and
-# their new sha1s.  The rewritten-pending list keeps the sha1s of
-# commits that have been processed, but not committed yet,
-# e.g. because they are waiting for a 'squash' command.
-rewritten_list="$state_dir"/rewritten-list
-rewritten_pending="$state_dir"/rewritten-pending
-
-# Work around Git for Windows' Bash whose "read" does not strip CRLF
-# and leaves CR at the end instead.
-cr=$(printf "\015")
-
-strategy_args=${strategy:+--strategy=$strategy}
-test -n "$strategy_opts" &&
-eval '
-   for strategy_opt in '"$strategy_opts"'
-   do
-   strategy_args="$strategy_args -X$(git rev-parse --sq-quote 
"${strategy_opt#--}")"
-   done
-'
-
 GIT_CHERRY_PICK_HELP="$resolvemsg"
 export GIT_CHERRY_PICK_HELP
 
@@ -105,15 +28,6 @@ case "$comment_char" in
;;
 esac
 
-warn () {
-   printf '%s\n' "$*" >&2
-}
-
-# Output the commit message for the specified commit.
-commit_message () {
-   git cat-file commit "$1" | sed "1,/^$/d"
-}
-
 orig_reflog_action="$GIT_REFLOG_ACTION"
 
 comment_for_reflog () {
@@ -125,33 +39,6 @@ comment_for_reflog () {
esac
 }
 
-last_count=
-mark_action_done () {
-   sed -e 1q < "$todo" >> "$done"
-   sed -e 1d < "$todo" >> "$todo".new
-   mv -f "$todo".new "$todo"
-   new_count=$(( $(git stripspace --strip-comments <"$done" | wc -l) ))
-   echo $new_count >"$msgnum"
-   total=$(($new_count + $(git stripspace --strip-comments <"$todo" | wc 
-l)))
-   echo $total >"$end"
-   if test "$last_count" != "$new_count"
-   then
-   last_count=$new_count
-   eval_gettext "Rebasing 

[GSoC][PATCH v4 0/4] rebase: split rebase -p from rebase -i

[Alban Gruin]

This splits the `rebase --preserve-merges` functionnality from
git-rebase--interactive.sh. All the dead code left by the duplication of
git-rebase--interactive.sh is also removed. This will make it easier to rewrite
this script in C.

This patch series is based of js/sequencer-and-root-commits.

Changes since v3:

 - Rewording the commits to be more descriptive, and to respect the 72
   characters limit.

Alban Gruin (4):
  rebase: introduce a dedicated backend for --preserve-merges
  rebase: strip unused code in git-rebase--preserve-merges.sh
  rebase: use the new git-rebase--preserve-merges.sh
  rebase: remove -p code from git-rebase--interactive.sh

 .gitignore |1 +
 Makefile   |2 +
 git-rebase--interactive.sh |  802 +--
 git-rebase--preserve-merges.sh | 1012 
 git-rebase.sh  |   32 +-
 5 files changed, 1048 insertions(+), 801 deletions(-)
 create mode 100644 git-rebase--preserve-merges.sh

-- 
2.16.1

protocol for updating .po gettext files?

[Robert P. J. Day]

   i was going to submit a minor patch to fix grammar here:

builtin/init-db.c: warning(_("templates not found %s"), template_dir);

as it should display "templates not found in %s" to be consistent with
other messages, but i know from nothing about .po files, so does one
also have to update those, or how does that work?

rday

-- 


Robert P. J. Day Ottawa, Ontario, CANADA
  http://crashcourse.ca/dokuwiki

Twitter:   http://twitter.com/rpjday
LinkedIn:   http://ca.linkedin.com/in/rpjday

Re: js/empty-config-section-fix, was Re: What's cooking in git.git (May 2018, #03; Wed, 23)

[Johannes Schindelin]

Hi Junio,

On Mon, 28 May 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > On Thu, 24 May 2018, Junio C Hamano wrote:
> >
> >> * js/empty-config-section-fix (2018-05-18) 1 commit
> >>  - config: a user-provided invalid section is not a BUG
> >> 
> >>  Error codepath fix.
> >> 
> >>  Will merge to 'next'.
> >
> > As a hotfix, maybe we can fast-track it to master?
> 
> Hotfix is a proposed fix for an issue that is so important to be a
> showstopper.  This one must be fixed before the final release, but I
> do not think it's not that urgent to force us to drop everything
> else and merge it to master immediately.

Well, in this case, a user might report a BUG when they simply have an
invalid config.

I do not care all that much, but Peff (who reported this) might.

Ciao,
Dscho

URGENT BSINESS ASSISTANT NEEDED

[Mr Usman Hassan]

Dear Friend


You may be surprise to receive this mail since you don’t know me
personally, but with due respect, trust and humility, I write to you
this proposal. I am Mr. us man Hassan the son of Mr. Ahmed   Hassan of
Darfur Sohut Sudan. It is indeed my pleasure to contact you for
assistance of a business venture which I intend to establish in a
country with a stable economy.

 I got your contact while I was doing a private research on the
Internet for a reliable and capable foreign partner that will assist
me and my family to transfer a fund to a personal or private account
for investment purpose.  Though I have not met with you before, but
considering the recent political instabilities in my country, I
believe one has to risk confiding in success sometimes in life.


There is this huge amount of money ($ 25, million US dollars) which my
late Father deposited here in Sohut Africa awaiting claim before he
was assassinated by unknown persons during this war in Darfur Sudan.


Now I have decided to invest this money in a stable economy country or
anywhere safe for security and political reasons. I want you to help
me retrieve this money for onward transfer to any designated bank
account of your choice for investment purposes on these areas below:

1) Transport Industry

2) Mechanized agriculture.

3} Estate investment

I will then furnish you with more details and I have mutually agreed
to compensate you with 30% which is your share for assisting me, and
5% for any expenses that might be incurred by both parties in the
course of the transaction. Then the remaining 65% will be for me and
my family, which you will help us to invest in your country.

Please, you can contact me through this Email=
usmanhassa...@hotmail.com all require is your honest & kind
co-operation.

I will give you further details as soon as you show interest in helping me.

I wait for your kind consideration to my proposal.

Best Regards,

Mr. Us man Hassan

Re: [PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> On Thu, May 24, 2018 at 9:02 PM, Jeff King  wrote:
>> On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:
>>
>>> That doesn't work, because that's for the server-side, but I need the
>>> fetch.fsck.* that doesn't exist. This works (I'll send a better patch
>>> with tests / docs etc. soon):
>>
>> Yeah, I think this is the right direction.

It seems that this 4-patch series is sufficiently commented and
earlier parts can be further polished soonish using help made by
others.  I need to re-read the last patch (implementation) but I
think this generally is good.

Thanks.

Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*

[Junio C Hamano]

Eric Sunshine  writes:

>> @@ -1554,23 +1554,42 @@ filter..smudge::
>> +Depending on the circumstances it might be better to use
>> +`fsck.skipList` instead to explicitly whitelist those objects that
>> +have issues, otherwise new occurrences of the same issue will be

I had to read the "instead to ..." part three times before
convincing myself that this is a good description, and I agree with
the assessment.  Perhaps we would want to make the recommendation a
bit stronger, even.

In general, it is better to enumerate existing objects with
problems with skipList, instead of listing the kind of
breakages these problematic objects share to be ignored, as
doing the latter will allow new instances of the same
breakages go unnoticed.

If the project has some tool constraints and have to accept new
"broken" objects on ongoing basis, then fsck. facility may
make sense, but that is probably a very narrow special use case.

>> +hidden going forward, although that's unlikely to happen in practice
>> +unless someone is being deliberately malicious.
>
> Is it worth mentioning buggy tools and/or other buggy Git
> implementations as sources?

Or old Git implementations we ourselves shipped.  I do not think it
is worth mentioning it, but I do agree that "unless somebody is
being deliberatly malicious" is misleading, if that is what you are
getting at.  One use of fsck is about noticing that you are under
attack, so "unless someone is being malicious" there in the sentence
does not make sense.  When somebody is attacking you, you do not
want to use fsck. to ignore it.

But that becomes a moot point, if we were to follow the line of
rewrite I suggested above.

[PATCH 2/2] show-index: update documentation for index v2

[Jeff King]

Commit 32637cdf4a (show-index.c: learn about index v2,
2007-04-09) changed the output format of show-index to
include the object CRC32 but didn't update the
documentation. Let's fix that and generally describe the
output in more detail.

There are a few other fixes here while we're rewording:

 - refer to index-pack along with pack-objects, since either
   can create .idx files

 - use "linkgit:" for referring to other commands

 - expand the bit about verify-pack, giving reasons why you
   might want this command instead. I almost omitted this
   entirely, but referring to verify-pack might help a
   reader who is looking for more information.

Signed-off-by: Jeff King 
---
 Documentation/git-show-index.txt | 26 --
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-show-index.txt b/Documentation/git-show-index.txt
index a8a9509e0e..424e4ba84c 100644
--- a/Documentation/git-show-index.txt
+++ b/Documentation/git-show-index.txt
@@ -14,13 +14,27 @@ SYNOPSIS
 
 DESCRIPTION
 ---
-Read the idx file for a Git packfile created with
-'git pack-objects' command from the standard input, and
-dump its contents.
+Read the `.idx` file for a Git packfile (created with
+linkgit:git-pack-objects[1] or linkgit:git-index-pack[1]) from the
+standard input, and dump its contents. The output consists of one object
+per line, with each line containing two or three space-separated
+columns:
 
-The information it outputs is subset of what you can get from
-'git verify-pack -v'; this command only shows the packfile
-offset and SHA-1 of each object.
+  - the first column is the offset in bytes of the object within the
+corresponding packfile
+
+  - the second column is the object id of the object
+
+  - if the index version is 2 or higher, the third column contains the
+CRC32 of the object data
+
+The objects are output in the order in which they are found in the index
+file, which should be (in a correctly constructed file) sorted by object
+id.
+
+Note that you can get more information on a packfile by calling
+linkgit:git-verify-pack[1]. However, as this command considers only the
+index file itself, it's both faster and more flexible.
 
 GIT
 ---
-- 
2.17.0.1391.g6fdbf40724

[PATCH 1/2] make show-index a builtin

[Jeff King]

The git-show-index command is built as its own separate
program. There's really no good reason for this, and it
means we waste extra space on disk (and CPU time running the
linker). Let's fold it in to the main binary as a builtin.

The history here is actually a bit amusing. The program
itself is mostly self-contained, and doesn't even use our
normal pack index code. In a5031214c4 (slim down "git
show-index", 2010-01-21), we even stopped using xmalloc() so
that it could avoid libgit.a entirely. But then 040a655116
(cleanup: use internal memory allocation wrapper functions
everywhere, 2011-10-06) switched that back to xmalloc, which
later become ALLOC_ARRAY().

Making it a builtin should give us the best of both worlds:
no wasted space and no need to avoid the usual patterns.

Signed-off-by: Jeff King 
---
 Makefile | 2 +-
 builtin.h| 1 +
 show-index.c => builtin/show-index.c | 2 +-
 git.c| 1 +
 4 files changed, 4 insertions(+), 2 deletions(-)
 rename show-index.c => builtin/show-index.c (96%)

diff --git a/Makefile b/Makefile
index ad880d1fc5..766c5909bf 100644
--- a/Makefile
+++ b/Makefile
@@ -689,7 +689,6 @@ PROGRAM_OBJS += http-backend.o
 PROGRAM_OBJS += imap-send.o
 PROGRAM_OBJS += sh-i18n--envsubst.o
 PROGRAM_OBJS += shell.o
-PROGRAM_OBJS += show-index.o
 PROGRAM_OBJS += remote-testsvn.o
 
 # Binary suffix, set to .exe for Windows builds
@@ -1076,6 +1075,7 @@ BUILTIN_OBJS += builtin/send-pack.o
 BUILTIN_OBJS += builtin/serve.o
 BUILTIN_OBJS += builtin/shortlog.o
 BUILTIN_OBJS += builtin/show-branch.o
+BUILTIN_OBJS += builtin/show-index.o
 BUILTIN_OBJS += builtin/show-ref.o
 BUILTIN_OBJS += builtin/stripspace.o
 BUILTIN_OBJS += builtin/submodule--helper.o
diff --git a/builtin.h b/builtin.h
index 4e0f64723e..0362f1ce25 100644
--- a/builtin.h
+++ b/builtin.h
@@ -220,6 +220,7 @@ extern int cmd_serve(int argc, const char **argv, const 
char *prefix);
 extern int cmd_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_show_index(int argc, const char **argv, const char *prefix);
 extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_submodule__helper(int argc, const char **argv, const char 
*prefix);
diff --git a/show-index.c b/builtin/show-index.c
similarity index 96%
rename from show-index.c
rename to builtin/show-index.c
index 1ead41e211..65fa86dd08 100644
--- a/show-index.c
+++ b/builtin/show-index.c
@@ -4,7 +4,7 @@
 static const char show_index_usage[] =
 "git show-index";
 
-int cmd_main(int argc, const char **argv)
+int cmd_show_index(int argc, const char **argv, const char *prefix)
 {
int i;
unsigned nr;
diff --git a/git.c b/git.c
index 5771d62a32..c91e144d9a 100644
--- a/git.c
+++ b/git.c
@@ -470,6 +470,7 @@ static struct cmd_struct commands[] = {
{ "shortlog", cmd_shortlog, RUN_SETUP_GENTLY | USE_PAGER },
{ "show", cmd_show, RUN_SETUP },
{ "show-branch", cmd_show_branch, RUN_SETUP },
+   { "show-index", cmd_show_index },
{ "show-ref", cmd_show_ref, RUN_SETUP },
{ "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
-- 
2.17.0.1391.g6fdbf40724

[PATCH 0/2] minor show-index modernizations

[Jeff King]

While recommending show-index to somebody today, I noticed that it has a
few ancient warts. This series turns it into a builtin and fixes the
documentation. I suspect it could do with some more modernization and
friendliness, like:

  - re-using the existing packfile.c code instead of re-implementing
index-reading (though the packfile code would definitely need some
refactoring to make this work)

  - it could probably handle arguments as files instead of requiring
stdin redirection

I'll leave those for now, as they're not worth the effort to me at this
point. But everybody who _doesn't_ use the command benefits from making
it a builtin, so it seems like an easy win.

  [1/2]: make show-index a builtin
  [2/2]: show-index: update documentation for index v2

 Documentation/git-show-index.txt | 22 --
 Makefile |  2 +-
 builtin.h|  1 +
 show-index.c => builtin/show-index.c |  2 +-
 git.c|  1 +
 5 files changed, 20 insertions(+), 8 deletions(-)
 rename show-index.c => builtin/show-index.c (96%)

-Peff

Re: [RFC PATCH 2/3] usage: prefix all lines in `vreportf()`, not just the first

[Junio C Hamano]

Martin Ågren  writes:

> diff --git a/t/t1011-read-tree-sparse-checkout.sh 
> b/t/t1011-read-tree-sparse-checkout.sh
> index 0c6f48f302..31b0702e6c 100755
> --- a/t/t1011-read-tree-sparse-checkout.sh
> +++ b/t/t1011-read-tree-sparse-checkout.sh
> @@ -243,9 +243,9 @@ test_expect_success 'print errors when failed to update 
> worktree' '
>   test_must_fail git checkout top 2>actual &&
>   cat >expected <<\EOF &&
>  error: The following untracked working tree files would be overwritten by 
> checkout:
> - sub/added
> - sub/addedtoo
> -Please move or remove them before you switch branches.
> +error:   sub/added
> +error:   sub/addedtoo
> +error: Please move or remove them before you switch branches.
>  Aborting
>  EOF

This shows the typical effect of this series, which (I subjectively
think) gives us a more pleasant end-user experience.

> diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
> index 4ee009da66..80d35087b7 100755
> --- a/t/t1506-rev-parse-diagnosis.sh
> +++ b/t/t1506-rev-parse-diagnosis.sh
> @@ -11,7 +11,7 @@ test_did_you_mean ()
>   sq="'" &&
>   cat >expected <<-EOF &&
>   fatal: Path '$2$3' $4, but not ${5:-$sq$3$sq}.
> - Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
> + fatal: Did you mean '$1:$2$3'${2:+ aka $sq$1:./$3$sq}?
>   EOF

And this, too.

> diff --git a/usage.c b/usage.c
> index 80f9c1d14b..6a5669922f 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -34,7 +34,7 @@ void vreportf(const char *prefix, const char *err, va_list 
> params)
>   if (iscntrl(*p) && *p != '\t' && *p != '\n')
>   *p = '?';
>   }
> - fprintf(stderr, "%s%s\n", prefix, msg);
> + prefix_suffix_lines(stderr, prefix, msg, "");
>  }

[RFC PATCH v4] Implement --first-parent for git rev-list --bisect

[Tiago Botelho]

This will enable users to implement bisecting on first parents
which can be useful for when the commits from a feature branch
that we want to merge are not always tested.

Signed-off-by: Tiago Botelho 
---

This patch adds all Junio's suggestions, namely do_find_bisection() being
broken when assigning q's weight to p if in first parent mode and q being
not UNINTERESTING and its weight still not being known.

The graph displayed in the unit tests was also changed from being top-bottom
to be left-right in order to keep it consistent with graphs in other tests.

 bisect.c   | 45 ++---
 bisect.h   |  3 ++-
 builtin/rev-list.c |  3 +++
 revision.c |  3 ---
 t/t6002-rev-list-bisect.sh | 37 +
 5 files changed, 72 insertions(+), 19 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4eafc8262..e58cb8d62 100644
--- a/bisect.c
+++ b/bisect.c
@@ -33,6 +33,8 @@ static const char *term_good;
  *
  * We care just barely enough to avoid recursing for
  * non-merge entries.
+ *
+ * Note: This function does not support the usage --first-parent.
  */
 static int count_distance(struct commit_list *entry)
 {
@@ -82,15 +84,16 @@ static inline void weight_set(struct commit_list *elem, int 
weight)
*((int*)(elem->item->util)) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, unsigned 
bisect_flags)
 {
struct commit_list *p;
int count;
 
for (count = 0, p = commit->parents; p; p = p->next) {
-   if (p->item->object.flags & UNINTERESTING)
-   continue;
-   count++;
+   if (!(p->item->object.flags & UNINTERESTING))
+   count++;
+   if (bisect_flags & BISECT_FIRST_PARENT)
+   break;
}
return count;
 }
@@ -117,10 +120,10 @@ static inline int halfway(struct commit_list *p, int nr)
 }
 
 #if !DEBUG_BISECT
-#define show_list(a,b,c,d) do { ; } while (0)
+#define show_list(a,b,c,d,e) do { ; } while (0)
 #else
 static void show_list(const char *debug, int counted, int nr,
- struct commit_list *list)
+ struct commit_list *list, unsigned bisect_flags)
 {
struct commit_list *p;
 
@@ -146,10 +149,14 @@ static void show_list(const char *debug, int counted, int 
nr,
else
fprintf(stderr, "---");
fprintf(stderr, " %.*s", 8, oid_to_hex(>object.oid));
-   for (pp = commit->parents; pp; pp = pp->next)
+   for (pp = commit->parents; pp; pp = pp->next) {
fprintf(stderr, " %.*s", 8,
oid_to_hex(>item->object.oid));
 
+   if (bisect_flags & BISECT_FIRST_PARENT)
+   break;
+   }
+
subject_len = find_commit_subject(buf, _start);
if (subject_len)
fprintf(stderr, " %.*s", subject_len, subject_start);
@@ -267,13 +274,13 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
unsigned flags = commit->object.flags;
 
p->item->util = [n++];
-   switch (count_interesting_parents(commit)) {
+   switch (count_interesting_parents(commit, bisect_flags)) {
case 0:
if (!(flags & TREESAME)) {
weight_set(p, 1);
counted++;
show_list("bisection 2 count one",
- counted, nr, list);
+ counted, nr, list, bisect_flags);
}
/*
 * otherwise, it is known not to reach any
@@ -289,7 +296,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
}
}
 
-   show_list("bisection 2 initialize", counted, nr, list);
+   show_list("bisection 2 initialize", counted, nr, list, bisect_flags);
 
/*
 * If you have only one parent in the resulting set
@@ -319,7 +326,7 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
counted++;
}
 
-   show_list("bisection 2 count_distance", counted, nr, list);
+   show_list("bisection 2 count_distance", counted, nr, list, 
bisect_flags);
 
while (counted < nr) {
for (p = list; p; p = p->next) {
@@ -329,6 +336,11 @@ static struct commit_list *do_find_bisection(struct 
commit_list *list,
if (0 <= weight(p))
continue;
for (q = p->item->parents; q; q = q->next) {
+   if 

Re: [RFC PATCH 1/3] usage: extract `prefix_suffix_lines()` from `advise()`

[Junio C Hamano]

Martin Ågren  writes:

> I'm open for suggestions on the naming of `prefix_suffix_lines()`...

Is there a verb that means "have/place the thing in between two
other things" or "Bring two things and place them on each side of
the third thing" in a more concise way?  Wrap?  Sandwich?  Enclose?

> +
> +/*
> + * Write the message to the file, prefixing and suffixing
> + * each line with `prefix` resp. `suffix`.
> + */
> +void prefix_suffix_lines(FILE *f, const char *prefix,
> +  const char *message, const char *suffix);
> +
> ...
> diff --git a/usage.c b/usage.c
> index cdd534c9df..80f9c1d14b 100644
> --- a/usage.c
> +++ b/usage.c
> @@ -6,6 +6,24 @@
>  #include "git-compat-util.h"
>  #include "cache.h"
>  
> +void prefix_suffix_lines(FILE *f,
> +  const char *prefix,
> +  const char *message,
> +  const char *suffix)
> +{
> + const char *cp, *np;
> +
> + for (cp = message; *cp; cp = np) {
> + np = strchrnul(cp, '\n');
> + fprintf(f, "%s%.*s%s\n",
> + prefix,
> + (int)(np - cp), cp,
> + suffix);
> + if (*np)
> + np++;
> + }
> +}
> +
>  void vreportf(const char *prefix, const char *err, va_list params)
>  {
>   char msg[4096];

I guess we can directly use this even in the codepath that
implements die() without having to worry about the helper making any
extra allocation, which is good.