Re: storing cover letter of a patch series?

[Jacob Keller]

On Mon, Aug 15, 2016 at 8:45 PM, Duy Nguyen  wrote:
> On Tue, Aug 16, 2016 at 3:46 AM, Philip Oakley  wrote:
>> From: "Jacob Keller" 
>> [nip]


 I've no problem with more extensive methods for those preparing very big
 patch series, or with those needing to merge together a lot of series and
 want to keep the cover letters, but ensuring that a simple flow is
 possible
 should still be there.
 --
 Philip

>>>
>>> Some people have suggested this simple idea, and I like it, but they
>>> did mention that modifying the cover letter now requires a rebase over
>>> a potentially large series of patches, which can get annoying.
>>>
>>> Thanks,
>>> Jake
>>
>>
>> They can just add "squash! cover! " commits for that ;-) Though more
>> likely the advanced workflow would be used... We'll need both (more than
>> one) options.
>
> Or even better, "git commit --reword $SHA1" brings up the editor with
> commit message of $SHA1. Modify any way you want and it creates a new
> empty, "reword!" commit that contains the diff between the old commit
> message and the new one. "reword!" can be consumed by "rebase -i
> --autosquash" without bringing up the editor again. I realize making
> "git commit --reword" run multiple times would be tricky though...
> --
> Duy

I was just thinking you write text and it gets appended to the text of
the reworded commit, and when you squash them using rebase you get to
finalize it like a normal squash?

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

Re: Minor bug: git config ignores empty sections

[Eli Barzilay]

On Mon, Aug 15, 2016 at 2:55 PM, Jeff King  wrote:
> On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:
>
>> I notice that we have thought about all the issues when we last
>> discussed it in 2013.  Refining a message from the earlier thread,
>> as it illustrates tricky cases in which we have to be careful.
>
> Thanks for digging up the threads that I was too lazy to find.
>
> I agree with most everything here, though I would be happy if somebody
> even wrote a patch to handle the "easy" cases.

So it sounds like removing an empty header is problematic in most cases,
but adding a new variable to an existing empty header should not be...?

I looked at the code, and had a rough sketch that works as follows:

* Make git_parse_source() call the callback in a special way to note
  that a section header is seen (I hacked it by passing a special value,
  a pointer to a global string, as the second argument)

* Add a new store.last_section_offset field

* In store_aux(), if it's getting the special value, and the section
  name matches, save the offset in store.last_section_offset

* In git_config_set_multivar_in_file_gently() right before the "write
  the first part of the config" comment, test that
  (store.seen == 1 && copy_begin == 0 && copy_end == contents_sz
   && store.last_section_offset > 0)
  and if so, write the contents up to that point, and set copy_begin;
  if the condition is false, do the same thing it does now

* A bit after that, add "&& store.last_section_offset == 0" to the
  condition that decides whether to call store_write_section()

It looks to me like something like this can work, but it's very hacky
because I wanted to see if it can work quickly, and because I don't know
if this kind of a solution will be wanted, and because I don't know
enough about that code in general.  Making it be an actual solution
involves a better way to call the callback in a special way (my hack
does the special thing only if `fn==store_aux`, but it shouldn't),
calling it after the last variable in a section is seen so it's added
after that.

So, will something like this be acceptable?  If so, is there anyone who
I can ask questions about the code?

-- 
   ((x=>x(x))(x=>x(x)))  Eli Barzilay:
   http://barzilay.org/  Maze is Life!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Duy Nguyen]

On Tue, Aug 16, 2016 at 3:46 AM, Philip Oakley  wrote:
> From: "Jacob Keller" 
> [nip]
>>>
>>>
>>> I've no problem with more extensive methods for those preparing very big
>>> patch series, or with those needing to merge together a lot of series and
>>> want to keep the cover letters, but ensuring that a simple flow is
>>> possible
>>> should still be there.
>>> --
>>> Philip
>>>
>>
>> Some people have suggested this simple idea, and I like it, but they
>> did mention that modifying the cover letter now requires a rebase over
>> a potentially large series of patches, which can get annoying.
>>
>> Thanks,
>> Jake
>
>
> They can just add "squash! cover! " commits for that ;-) Though more
> likely the advanced workflow would be used... We'll need both (more than
> one) options.

Or even better, "git commit --reword $SHA1" brings up the editor with
commit message of $SHA1. Modify any way you want and it creates a new
empty, "reword!" commit that contains the diff between the old commit
message and the new one. "reword!" can be consumed by "rebase -i
--autosquash" without bringing up the editor again. I realize making
"git commit --reword" run multiple times would be tricky though...
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] make rebase respect core.hooksPath if set

[ryenus]

On 15 August 2016 at 20:24, Johannes Schindelin
 wrote:

> Would it not be more appropriate to teach --git-path hooks to respect the
> core.hooksPath variable? This would be in line with --git-path objects
> respecting the GIT_OBJECT_DIRECTORY environment variable.

Indeed, I've thought about that, too, due to the bad smell of duplication,
but not sure if that's the right position among all the abstraction layers.

Also it's more convenient to come up with a shell based fix on local
installation.

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

Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Mon, Aug 15, 2016 at 03:48:55PM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > The simple fix is to call register_tempfile() in open_pack_file(), and
> > just have index-pack clean up the file on its way out.
> >
> > But there are harder cases. For instance, imagine somebody pushes a
> > 500MB file, and you have a pre-receive hook that says "too big; I won't
> > accept this". And then they push in a loop, as before. You've accepted
> > the incoming pack into the repository by the time the pre-receive runs.
> > You can't just delete it, because you don't know if other simultaneous
> > processes have started to depend on the objects.
> >
> > To solve that, I have patches that put incoming packfiles into a
> > "quarantine" area, then run the connectivity check and pre-receive hooks
> > with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
> > then we either move the quarantine packs into the real repo, or blow
> > away the tmpdir, depending on whether the hooks said the objects were
> > OK.
> >
> > Those are patches I plan to share upstream but just haven't gotten
> > around to yet.
> 
> I think these other patches can come later, independent from this
> three-patch series resurrected from the archive, so I can take a
> reroll of these once the integer-size issues you pointed out are
> sorted out.

Yeah, definitely it's independent. I was mostly suggesting to Christian
that he may want to look into the "easy" register_tempfile() case, as
GitLab may find this is only half of the fix that they want.

Also a patch I may try to polish and share in the future, but not as
high priority as some of the other stuff.

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

Re: [PATCH v5 2/3] diff: add --diff-line-prefix option for passing in a prefix

[Jacob Keller]

On Mon, Aug 15, 2016 at 8:37 AM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> I will look more into how to do the log version tomorrow, if I am
>> still stuck I will re-work the patches as you suggest here.
>>
>> I am hoping I can find a good solution for how to handle it though.
>
> Thanks, I am hoping the same, too ;-)

Ok, so I found a way that (ab)uses the graph API to enable line-prefix
support. I moved the way we handle diff prefix into the graph callback
handler as well, and I also moved around some code so that the graph
API will work correctly. I found several places that actually broke if
you tried calling the graph API with a null graph, because they would
still try to access the FILE* pointer inside and could lead to a
segfault. This should fix those up as well.

Hopefully this is along the lines of what you were thinking, and it
should allow more advanced calls to a submodule format that shows the
full log if we wanted.

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

[PATCH v6 1/3] diff.c: remove output_prefix_length field

[Jacob Keller]

From: Junio C Hamano 

"diff/log --stat" has a logic that determines the display columns
available for the diffstat part of the output and apportions it for
pathnames and diffstat graph automatically.

5e71a84a (Add output_prefix_length to diff_options, 2012-04-16)
added the output_prefix_length field to diff_options structure to
allow this logic subtract the display columns used for display the
history graph part from the total "terminal width"; this matters
when the "git log --graph -p" option is in use.

The field be set to the number of display columns needed to show the
output from the output_prefix() callback.  Any new output_prefix()
callback must also update the field accordingly, which is error
prone.  As there is only one user of the field, and the user has the
actual value of the prefix string, let's get rid of the field and
have the user count the display width itself.

Signed-off-by: Junio C Hamano 
---
 diff.c  | 2 +-
 diff.h  | 1 -
 graph.c | 2 --
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/diff.c b/diff.c
index b43d3dd2ecb7..ae069c303077 100644
--- a/diff.c
+++ b/diff.c
@@ -1625,7 +1625,7 @@ static void show_stats(struct diffstat_t *data, struct 
diff_options *options)
 */
 
if (options->stat_width == -1)
-   width = term_columns() - options->output_prefix_length;
+   width = term_columns() - strlen(line_prefix);
else
width = options->stat_width ? options->stat_width : 80;
number_width = decimal_width(max_change) > number_width ?
diff --git a/diff.h b/diff.h
index 125447be09eb..49e4aaafb2da 100644
--- a/diff.h
+++ b/diff.h
@@ -174,7 +174,6 @@ struct diff_options {
diff_format_fn_t format_callback;
void *format_callback_data;
diff_prefix_fn_t output_prefix;
-   int output_prefix_length;
void *output_prefix_data;
 
int diff_path_counter;
diff --git a/graph.c b/graph.c
index dd1720148dc5..a46803840511 100644
--- a/graph.c
+++ b/graph.c
@@ -197,7 +197,6 @@ static struct strbuf *diff_output_prefix_callback(struct 
diff_options *opt, void
assert(opt);
assert(graph);
 
-   opt->output_prefix_length = graph->width;
strbuf_reset();
graph_padding_line(graph, );
return 
@@ -245,7 +244,6 @@ struct git_graph *graph_init(struct rev_info *opt)
 */
opt->diffopt.output_prefix = diff_output_prefix_callback;
opt->diffopt.output_prefix_data = graph;
-   opt->diffopt.output_prefix_length = 0;
 
return graph;
 }
-- 
2.9.2.873.g47c31b4

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

[PATCH v6 2/3] graph: add support for --line-prefix on all graph-aware output

[Jacob Keller]

From: Jacob Keller 

Add an extension to git-diff and git-log (and any other graph-aware
displayable output) such that "--line-prefix=" will print the
additional line-prefix on every line of output.

To make this work, we have to fix a few bugs in the graph API that force
graph_show_commit_msg to be used only when you have a valid graph.
Additionally, we extend the default_diff_output_prefix handler to work
even when no graph is enabled.

This is somewhat of a hack on top of the graph API, but I think it
should be acceptable here.

This will be used by a future extension of submodule display which
displays the submodule diff as the actual diff between the pre and post
commit in the submodule project.

Add some tests for both git-log and git-diff to ensure that the prefix
is honored correctly.

Signed-off-by: Jacob Keller 
---
 Documentation/diff-options.txt |   3 +
 builtin/rev-list.c |  70 ++---
 diff.c |   6 +
 diff.h |   1 +
 graph.c| 103 ---
 graph.h|  22 +-
 log-tree.c |   5 +-
 t/t4013-diff-various.sh|   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 ++
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4202-log.sh | 323 +
 11 files changed, 503 insertions(+), 80 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a87394200..cc4342e2034d 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -569,5 +569,8 @@ endif::git-format-patch[]
 --no-prefix::
Do not show any source or destination prefix.
 
+--line-prefix=::
+   Prepend an additional prefix to every line of output.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0ba82b1635b6..1a75a83538f4 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -122,48 +122,40 @@ static void show_commit(struct commit *commit, void *data)
ctx.fmt = revs->commit_format;
ctx.output_encoding = get_log_output_encoding();
pretty_print_commit(, commit, );
-   if (revs->graph) {
-   if (buf.len) {
-   if (revs->commit_format != CMIT_FMT_ONELINE)
-   graph_show_oneline(revs->graph);
+   if (buf.len) {
+   if (revs->commit_format != CMIT_FMT_ONELINE)
+   graph_show_oneline(revs->graph);
 
-   graph_show_commit_msg(revs->graph, );
+   graph_show_commit_msg(revs->graph, stdout, );
 
-   /*
-* Add a newline after the commit message.
-*
-* Usually, this newline produces a blank
-* padding line between entries, in which case
-* we need to add graph padding on this line.
-*
-* However, the commit message may not end in a
-* newline.  In this case the newline simply
-* ends the last line of the commit message,
-* and we don't need any graph output.  (This
-* always happens with CMIT_FMT_ONELINE, and it
-* happens with CMIT_FMT_USERFORMAT when the
-* format doesn't explicitly end in a newline.)
-*/
-   if (buf.len && buf.buf[buf.len - 1] == '\n')
-   graph_show_padding(revs->graph);
-   putchar('\n');
-   } else {
-   /*
-* If the message buffer is empty, just show
-* the rest of the graph output for this
-* commit.
-*/
-   if (graph_show_remainder(revs->graph))
-   putchar('\n');
-   if (revs->commit_format == CMIT_FMT_ONELINE)
-   putchar('\n');
-   }
+   /*
+   

[PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

[Jacob Keller]

From: Jacob Keller 

Teach git-diff and friends a new format for displaying the difference of
a submodule using git-diff inside the submodule project. This allows
users to easily see exactly what source changed in a given commit that
updates the submodule pointer. To do this, remove DIFF_SUBMODULE_LOG bit
from the diff options, and instead add a new enum type for these
formats.

Add some tests for the new format and option.

Signed-off-by: Jacob Keller 
---
 Documentation/diff-config.txt|   3 +-
 Documentation/diff-options.txt   |   7 +-
 diff.c   |  41 +-
 diff.h   |   9 +-
 submodule.c  | 130 +
 submodule.h  |   6 +
 t/t4059-diff-submodule-option-diff-format.sh | 738 +++
 7 files changed, 915 insertions(+), 19 deletions(-)
 create mode 100755 t/t4059-diff-submodule-option-diff-format.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d5a5b17d5088..f5d693afad6c 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -123,7 +123,8 @@ diff.suppressBlankEmpty::
 diff.submodule::
Specify the format in which differences in submodules are
shown.  The "log" format lists the commits in the range like
-   linkgit:git-submodule[1] `summary` does.  The "short" format
+   linkgit:git-submodule[1] `summary` does.  The "diff" format shows the
+   diff between the beginning and end of the range. The "short" format
format just shows the names of the commits at the beginning
and end of the range.  Defaults to short.
 
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cc4342e2034d..d3ca4ad2c2c5 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -215,8 +215,11 @@ any of those replacements occurred.
the commits in the range like linkgit:git-submodule[1] `summary` does.
Omitting the `--submodule` option or specifying `--submodule=short`,
uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.  Can be tweaked via the
-   `diff.submodule` configuration variable.
+   at the beginning and end of the range. When `--submodule=diff` is
+   given, the 'diff' format is used. This format shows the diff between
+   the old and new submodule commmit from the perspective of the
+   submodule.  Defaults to `diff.submodule` or 'short' if the config
+   option is unset.
 
 --color[=]::
Show colored diff.
diff --git a/diff.c b/diff.c
index e286897b51e6..232fbe17680f 100644
--- a/diff.c
+++ b/diff.c
@@ -132,9 +132,11 @@ static int parse_dirstat_params(struct diff_options 
*options, const char *params
 static int parse_submodule_params(struct diff_options *options, const char 
*value)
 {
if (!strcmp(value, "log"))
-   DIFF_OPT_SET(options, SUBMODULE_LOG);
+   options->submodule_format = DIFF_SUBMODULE_LOG;
+   else if (!strcmp(value, "diff"))
+   options->submodule_format = DIFF_SUBMODULE_DIFF;
else if (!strcmp(value, "short"))
-   DIFF_OPT_CLR(options, SUBMODULE_LOG);
+   options->submodule_format = DIFF_SUBMODULE_SHORT;
else
return -1;
return 0;
@@ -2300,9 +2302,18 @@ static void builtin_diff(const char *name_a,
struct strbuf header = STRBUF_INIT;
const char *line_prefix = diff_line_prefix(o);
 
-   if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
-   (!one->mode || S_ISGITLINK(one->mode)) &&
-   (!two->mode || S_ISGITLINK(two->mode))) {
+   diff_set_mnemonic_prefix(o, "a/", "b/");
+   if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
+   a_prefix = o->b_prefix;
+   b_prefix = o->a_prefix;
+   } else {
+   a_prefix = o->a_prefix;
+   b_prefix = o->b_prefix;
+   }
+
+   if (o->submodule_format == DIFF_SUBMODULE_LOG &&
+   (!one->mode || S_ISGITLINK(one->mode)) &&
+   (!two->mode || S_ISGITLINK(two->mode))) {
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one->path ? one->path : 
two->path,
@@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
two->dirty_submodule,
meta, del, add, reset);
return;
+   } else if (o->submodule_format == DIFF_SUBMODULE_DIFF &&
+  (!one->mode || S_ISGITLINK(one->mode)) &&
+  (!two->mode || S_ISGITLINK(two->mode))) {
+   show_submodule_diff(o->file, one->path ? one->path : two->path,
+   

[PATCH v6 0/3] Add support for displaying submodules as a proper diff

[Jacob Keller]

From: Jacob Keller 

This patch series adds support for displaying a submodule as a
difference between the pre and post commits. This allows projects who
frequently update submodule contents to view the submodule in the log as
if it were just one squashed commit that updates all the files in the
submodule together, even if you mix-match the submodule part with a
regular part you just see a complete diff that shows all the changes.

To make this work, I also extended the graph-aware output with a
--line-prefix option. This option extends both diff and log to show a
prefix. It's a bit of a hack in someways, but it works for showing a
prefix both when graph is enabled and when its not. I think it works
quite well.

I added several tests to the line-prefix, and also a few tests for the
new submodule format.

I welcome comments on how to improve the graph line-prefix code, as well
as the actual submodule diff format.

Jacob Keller (2):
  graph: add support for --line-prefix on all graph-aware output
  diff: add SUBMODULE_DIFF format to display submodule diff

Junio C Hamano (1):
  diff.c: remove output_prefix_length field

 Documentation/diff-config.txt  |   3 +-
 Documentation/diff-options.txt |  10 +-
 builtin/rev-list.c |  70 +-
 diff.c |  49 +-
 diff.h |  11 +-
 graph.c| 105 +--
 graph.h|  22 +-
 log-tree.c |   5 +-
 submodule.c| 130 
 submodule.h|   6 +
 t/t4013-diff-various.sh|   6 +
 ...diff.diff_--line-prefix=abc_master_master^_side |  29 +
 t/t4013/diff.diff_--line-prefix_--cached_--_file0  |  15 +
 t/t4059-diff-submodule-option-diff-format.sh   | 738 +
 t/t4202-log.sh | 323 +
 15 files changed, 1419 insertions(+), 103 deletions(-)
 create mode 100644 t/t4013/diff.diff_--line-prefix=abc_master_master^_side
 create mode 100644 t/t4013/diff.diff_--line-prefix_--cached_--_file0
 create mode 100755 t/t4059-diff-submodule-option-diff-format.sh

-- 
2.9.2.873.g47c31b4

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

Re: storing cover letter of a patch series?

[Jacob Keller]

On Mon, Aug 15, 2016 at 1:38 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>> Some people have suggested this simple idea, and I like it, but they
>> did mention that modifying the cover letter now requires a rebase over
>> a potentially large series of patches, which can get annoying.
>
> That can be simply solved by keeping the cover at the end.  When you
> are updating the real patch on the series with "rebase -i", you
> would have a chance to update the cover at the same time that way.
>

It has problems keeping it at the end as well because that makes
regular commits and commit --amend funky.., but you could do squashes
into the cover letter easily enough as suggested by Philip Oakley. I
think that might be the most natural flow we have now that doesn't
depend on creating some fancy new object type.

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

Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Junio C Hamano]

Jeff King  writes:

> The simple fix is to call register_tempfile() in open_pack_file(), and
> just have index-pack clean up the file on its way out.
>
> But there are harder cases. For instance, imagine somebody pushes a
> 500MB file, and you have a pre-receive hook that says "too big; I won't
> accept this". And then they push in a loop, as before. You've accepted
> the incoming pack into the repository by the time the pre-receive runs.
> You can't just delete it, because you don't know if other simultaneous
> processes have started to depend on the objects.
>
> To solve that, I have patches that put incoming packfiles into a
> "quarantine" area, then run the connectivity check and pre-receive hooks
> with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
> then we either move the quarantine packs into the real repo, or blow
> away the tmpdir, depending on whether the hooks said the objects were
> OK.
>
> Those are patches I plan to share upstream but just haven't gotten
> around to yet.

I think these other patches can come later, independent from this
three-patch series resurrected from the archive, so I can take a
reroll of these once the integer-size issues you pointed out are
sorted out.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Jeff King  writes:

>> REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
>>  if (!opts->quiet) {
>>  if (old->path &&
>> -(advice_detached_head == 1 ||
>> - (advice_detached_head == -1 && 
>> !opts->force_detach)))
>> +advice_detached_head == 1 && !opts->force_detach)
>
> Maybe s/== 1//?

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

Re: [PATCHv5 0/8] git clone: Marry --recursive and --reference

[Junio C Hamano]

Stefan Beller  writes:

> As the comments only address programming work as opposed to design, the 
> interdiff is rather small:

Thanks.  Will queue.  I think we do not need that extra err strbuf
in add_one_reference(), but we can go either way and it is not worth
rerolling.

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

Re: [PATCH v2] difftool: always honor fatal error exit codes

[Junio C Hamano]

John Keeping  writes:

> Here's what that looks like.

Sounds good.  It feels a bit funny to see that new mentions of
$status are unquoted (which is totally valid because we know it has
$? that cannot be anything other than a short decimal integer),
while the one in the post-context quotes it, but that's not a huge
issue.

Will queue.  Thanks.

>  git-difftool--helper.sh | 7 +++
>  t/t7800-difftool.sh | 6 ++
>  2 files changed, 13 insertions(+)
>
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 84d6cc0..7bfb673 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -86,6 +86,13 @@ else
>   do
>   launch_merge_tool "$1" "$2" "$5"
>   status=$?
> + if test $status -ge 126
> + then
> + # Command not found (127), not executable (126) or
> + # exited via a signal (>= 128).
> + exit $status
> + fi
> +
>   if test "$status" != 0 &&
>   test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
>   then
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 2974900..70a2de4 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with 
> --trust-exit-code' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success PERL 'difftool honors exit status if command not found' '
> + test_config difftool.nonexistent.cmd i-dont-exist &&
> + test_config difftool.trustExitCode false &&
> + test_must_fail git difftool -y -t nonexistent branch
> +'
> +
>  test_expect_success PERL 'difftool honors --gui' '
>   difftool_test_setup &&
>   test_config merge.tool bogus-tool &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Junio C Hamano]

"Philip Oakley"  writes:

> The other option is to simply build a guide-list in exactly the same
> format as the command list (which if it works can be merged
> later). Re-use the existing code, etc.

Yeah, that sounds like a good way to go forward.  To implement typo
correction for "git help ", having guide-list would be
very useful.

A related tangent is that I think "git  --help" shouldn't
fall back to "git help ", regardless of typo correction.  It
happens to "work" only because we blindly turned " --help" to
"help " without even checking what  is.  Making it stop
"working" would be a bugfix.

And having both command and guide list would be helpful to prevent
"git  --help" from falling back to "git help ".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] difftool: always honor fatal error exit codes

[John Keeping]

At the moment difftool's "trust exit code" logic always suppresses the
exit status of the diff utility we invoke.  This is useful because we
don't want to exit just because diff returned "1" because the files
differ, but it's confusing if the shell returns an error because the
selected diff utility is not found.

POSIX specifies 127 as the exit status for "command not found", 126 for
"command found but is not executable" and values greater than 128 if the
command terminated because it received a signal [1] and at least bash
and dash follow this specification, while diff utilities generally use
"1" for the exit status we want to ignore.

Handle any value of 126 or greater as a special value indicating that
some form of fatal error occurred.

[1] 
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Signed-off-by: John Keeping 
---
On Mon, Aug 15, 2016 at 10:35:26PM +0100, John Keeping wrote:
> On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> > "Tom Tanner (BLOOMBERG/ LONDON)"  writes:
> > 
> > > From: gits...@pobox.com
> > > To: j...@keeping.me.uk
> > > Cc: Tom Tanner (BLOOMBERG/ LONDON), dav...@gmail.com, git@vger.kernel.org
> > > At: 08/14/16 04:21:18
> > >
> > > John Keeping  writes:
> > > ...
> > >> POSIX specifies 127 as the exit status for "command not found" and 126
> > >> for "command found but is not executable" [1] and at least bash and dash
> > >> follow this specification, while diff utilities generally use "1" for
> > >> the exit status we want to ignore.
> > >>
> > >> Handle 126 and 127 as special values, assuming that they always mean
> > >> that the command could not be executed.
> > >
> > > Sounds like a reasonable thing to do.  Will queue; thanks.
> > 
> > > Would it be possible to also treat signals (128 and above) as
> > > 'special' values as well (as I've seen some merge tools self
> > > destruct like that from time to time)
> > 
> > Certainly, it feels safer to notice an unusual exit status code and
> > error out to force the user to take notice, but that reasoning
> > assumes that "128 and above" are noteworthy exceptions.
> 
> Reading further in POSIX:
> 
>   The exit status of a command that terminated because it received
>   a signal shall be reported as greater than 128.
> 
> I think if we accept the argument above about diff utilities generally
> using low numbers for the status values we're ignoring intentionally,
> then we can just treat any value above 125 as a fatal error.

Here's what that looks like.

 git-difftool--helper.sh | 7 +++
 t/t7800-difftool.sh | 6 ++
 2 files changed, 13 insertions(+)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 84d6cc0..7bfb673 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -86,6 +86,13 @@ else
do
launch_merge_tool "$1" "$2" "$5"
status=$?
+   if test $status -ge 126
+   then
+   # Command not found (127), not executable (126) or
+   # exited via a signal (>= 128).
+   exit $status
+   fi
+
if test "$status" != 0 &&
test "$GIT_DIFFTOOL_TRUST_EXIT_CODE" = true
then
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 2974900..70a2de4 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -124,6 +124,12 @@ test_expect_success PERL 'difftool stops on error with 
--trust-exit-code' '
test_cmp expect actual
 '
 
+test_expect_success PERL 'difftool honors exit status if command not found' '
+   test_config difftool.nonexistent.cmd i-dont-exist &&
+   test_config difftool.trustExitCode false &&
+   test_must_fail git difftool -y -t nonexistent branch
+'
+
 test_expect_success PERL 'difftool honors --gui' '
difftool_test_setup &&
test_config merge.tool bogus-tool &&
-- 
2.9.3.728.g30b24b4

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

[PATCHv5 5/8] clone: factor out checking for an alternate path

[Stefan Beller]

In a later patch we want to determine if a path is suitable as an
alternate from other commands than builtin/clone. Move the checking
functionality of `add_one_reference` to `compute_alternate_path` that is
defined in cache.h.

Signed-off-by: Stefan Beller 
---
 builtin/clone.c | 43 +++-
 cache.h |  1 +
 sha1_file.c | 76 +
 3 files changed, 86 insertions(+), 34 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index f044a8c..4cd3a66 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -282,44 +282,19 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-   char *ref_git;
-   const char *repo;
-   struct strbuf alternate = STRBUF_INIT;
-
-   /* Beware: read_gitfile(), real_path() and mkpath() return static 
buffer */
-   ref_git = xstrdup(real_path(item->string));
-
-   repo = read_gitfile(ref_git);
-   if (!repo)
-   repo = read_gitfile(mkpath("%s/.git", ref_git));
-   if (repo) {
-   free(ref_git);
-   ref_git = xstrdup(repo);
-   }
-
-   if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
-   char *ref_git_git = mkpathdup("%s/.git", ref_git);
-   free(ref_git);
-   ref_git = ref_git_git;
-   } else if (!is_directory(mkpath("%s/objects", ref_git))) {
-   struct strbuf sb = STRBUF_INIT;
-   if (get_common_dir(, ref_git))
-   die(_("reference repository '%s' as a linked checkout 
is not supported yet."),
-   item->string);
-   die(_("reference repository '%s' is not a local repository."),
-   item->string);
-   }
+   struct strbuf err = STRBUF_INIT;
+   struct strbuf sb = STRBUF_INIT;
+   char *ref_git = compute_alternate_path(item->string, );
 
-   if (!access(mkpath("%s/shallow", ref_git), F_OK))
-   die(_("reference repository '%s' is shallow"), item->string);
+   if (!ref_git)
+   die("%s", err.buf);
 
-   if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
-   die(_("reference repository '%s' is grafted"), item->string);
+   strbuf_addf(, "%s/objects", ref_git);
+   add_to_alternates_file(sb.buf);
 
-   strbuf_addf(, "%s/objects", ref_git);
-   add_to_alternates_file(alternate.buf);
-   strbuf_release();
free(ref_git);
+   strbuf_release();
+   strbuf_release();
return 0;
 }
 
diff --git a/cache.h b/cache.h
index b5f76a4..1087dca 100644
--- a/cache.h
+++ b/cache.h
@@ -1342,6 +1342,7 @@ extern struct alternate_object_database {
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
 extern void read_info_alternates(const char * relative_base, int depth);
+extern char *compute_alternate_path(const char *path, struct strbuf *err);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern int foreach_alt_odb(alt_odb_fn, void*);
diff --git a/sha1_file.c b/sha1_file.c
index cb571ac..0fe5aa3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -425,6 +425,82 @@ void add_to_alternates_file(const char *reference)
free(alts);
 }
 
+/*
+ * Compute the exact path an alternate is at and returns it. In case of
+ * error NULL is returned and the human readable error is added to `err`
+ * `path` may be relative and should point to $GITDIR.
+ * `err` must not be null.
+ */
+char *compute_alternate_path(const char *path, struct strbuf *err)
+{
+   char *ref_git = NULL;
+   const char *repo, *ref_git_s;
+   int seen_error = 0;
+
+   ref_git_s = real_path_if_valid(path);
+   if (!ref_git_s) {
+   seen_error = 1;
+   strbuf_addf(err, _("path '%s' does not exist"), path);
+   goto out;
+   } else
+   /*
+* Beware: read_gitfile(), real_path() and mkpath()
+* return static buffer
+*/
+   ref_git = xstrdup(ref_git_s);
+
+   repo = read_gitfile(ref_git);
+   if (!repo)
+   repo = read_gitfile(mkpath("%s/.git", ref_git));
+   if (repo) {
+   free(ref_git);
+   ref_git = xstrdup(repo);
+   }
+
+   if (!repo && is_directory(mkpath("%s/.git/objects", ref_git))) {
+   char *ref_git_git = mkpathdup("%s/.git", ref_git);
+   free(ref_git);
+   ref_git = ref_git_git;
+   } else if (!is_directory(mkpath("%s/objects", ref_git))) {
+   struct strbuf sb = STRBUF_INIT;
+   seen_error = 1;
+   if (get_common_dir(, ref_git)) {
+   strbuf_addf(err,
+   _("reference repository '%s' as a linked "
+

[PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Stefan Beller]

When `--recursive` and `--reference` is given, it is reasonable to
expect that the submodules are created with references to the submodules
of the given alternate for the superproject.

  An initial attempt to do this was presented to the mailing list, which
  used flags that are passed around ("--super-reference") that instructed
  the submodule clone to look for a reference in the submodules of the
  referenced superproject. This is not well thought out, as any further
  `submodule update` should also respect the initial setup.

  When a new  submodule is added to the superproject and the alternate
  of the superproject does not know about that submodule yet, we rather
  error out informing the user instead of being unclear if we did or did
  not use a submodules alternate.

To solve this problem introduce new options that store the configuration
for what the user wanted originally.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt   | 12 ++
 builtin/clone.c| 19 +
 builtin/submodule--helper.c| 87 ++
 t/t7408-submodule-reference.sh | 43 +
 4 files changed, 161 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index bc1c433..602f43a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2837,6 +2837,18 @@ submodule.fetchJobs::
in parallel. A value of 0 will give some reasonable default.
If unset, it defaults to 1.
 
+submodule.alternateLocation::
+   Specifies how the submodules obtain alternates when submodules are
+   cloned. Possible values are `no`, `superproject`.
+   By default `no` is assumed, which doesn't add references. When the
+   value is set to `superproject` the submodule to be cloned computes
+   its alternates location relative to the superprojects alternate.
+
+submodule.alternateErrorStrategy
+   Specifies how to treat errors with the alternates for a submodule
+   as computed via `submodule.alternateLocation`. Possible values are
+   `ignore`, `info`, `die`.
+
 tag.forceSignAnnotated::
A boolean to specify whether annotated tags created should be GPG 
signed.
If `--annotate` is specified on the command line, it takes
diff --git a/builtin/clone.c b/builtin/clone.c
index 0182665..404c5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
else
fprintf(stderr, _("Cloning into '%s'...\n"), dir);
}
+
+   if (option_recursive) {
+   if (option_required_reference.nr &&
+   option_optional_reference.nr)
+   die(_("clone --recursive is not compatible with "
+ "both --reference and --reference-if-able"));
+   else if (option_required_reference.nr) {
+   string_list_append(_config,
+   "submodule.alternateLocation=superproject");
+   string_list_append(_config,
+   "submodule.alternateErrorStrategy=die");
+   } else if (option_optional_reference.nr) {
+   string_list_append(_config,
+   "submodule.alternateLocation=superproject");
+   string_list_append(_config,
+   "submodule.alternateErrorStrategy=info");
+   }
+   }
+
init_db(option_template, INIT_DB_QUIET);
write_config(_config);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7096848..681b91d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -472,6 +472,90 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
return run_command();
 }
 
+struct submodule_alternate_setup {
+   const char *submodule_name;
+   enum SUBMODULE_ALTERNATE_ERROR_MODE {
+   SUBMODULE_ALTERNATE_ERROR_DIE,
+   SUBMODULE_ALTERNATE_ERROR_INFO,
+   SUBMODULE_ALTERNATE_ERROR_IGNORE
+   } error_mode;
+   struct string_list *reference;
+};
+#define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
+   SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
+
+static int add_possible_reference_from_superproject(
+   struct alternate_object_database *alt, void *sas_cb)
+{
+   struct submodule_alternate_setup *sas = sas_cb;
+
+   /* directory name, minus trailing slash */
+   size_t namelen = alt->name - alt->base - 1;
+   struct strbuf name = STRBUF_INIT;
+   strbuf_add(, alt->base, namelen);
+
+   /*
+* If the alternate object store is another repository, try the
+* standard layout with .git/modules//objects
+*/
+   if (ends_with(name.buf, ".git/objects")) {
+   char *sm_alternate;
+   

[PATCHv5 3/8] submodule--helper module-clone: allow multiple references

[Stefan Beller]

Allow users to pass in multiple references, just as clone accepts multiple
references as well.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6f6d67a..ce9b3e9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -442,7 +442,7 @@ static int module_name(int argc, const char **argv, const 
char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char 
*url,
-  const char *depth, const char *reference, int quiet)
+  const char *depth, struct string_list *reference, 
int quiet)
 {
struct child_process cp;
child_process_init();
@@ -453,8 +453,12 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
argv_array_push(, "--quiet");
if (depth && *depth)
argv_array_pushl(, "--depth", depth, NULL);
-   if (reference && *reference)
-   argv_array_pushl(, "--reference", reference, NULL);
+   if (reference->nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, reference)
+   argv_array_pushl(, "--reference",
+item->string, NULL);
+   }
if (gitdir && *gitdir)
argv_array_pushl(, "--separate-git-dir", gitdir, NULL);
 
@@ -470,13 +474,13 @@ static int clone_submodule(const char *path, const char 
*gitdir, const char *url
 
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-   const char *name = NULL, *url = NULL;
-   const char *reference = NULL, *depth = NULL;
+   const char *name = NULL, *url = NULL, *depth = NULL;
int quiet = 0;
FILE *submodule_dot_git;
char *p, *path = NULL, *sm_gitdir;
struct strbuf rel_path = STRBUF_INIT;
struct strbuf sb = STRBUF_INIT;
+   struct string_list reference = STRING_LIST_INIT_NODUP;
 
struct option module_clone_options[] = {
OPT_STRING(0, "prefix", ,
@@ -491,8 +495,8 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "url", ,
   N_("string"),
   N_("url where to clone the submodule from")),
-   OPT_STRING(0, "reference", ,
-  N_("string"),
+   OPT_STRING_LIST(0, "reference", ,
+  N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", ,
   N_("string"),
@@ -528,7 +532,7 @@ static int module_clone(int argc, const char **argv, const 
char *prefix)
if (!file_exists(sm_gitdir)) {
if (safe_create_leading_directories_const(sm_gitdir) < 0)
die(_("could not create directory '%s'"), sm_gitdir);
-   if (clone_submodule(path, sm_gitdir, url, depth, reference, 
quiet))
+   if (clone_submodule(path, sm_gitdir, url, depth, , 
quiet))
die(_("clone of '%s' into submodule path '%s' failed"),
url, path);
} else {
-- 
2.9.2.730.g525ad04.dirty

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

[PATCHv5 6/8] clone: clarify option_reference as required

[Stefan Beller]

In the next patch we introduce optional references; To better distinguish
between optional and required references we rename the variable.

Signed-off-by: Stefan Beller 
---
 builtin/clone.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 4cd3a66..ef5db7c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,7 +50,7 @@ static int option_verbosity;
 static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
-static struct string_list option_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -79,7 +79,7 @@ static struct option builtin_clone_options[] = {
N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", _template, N_("template-directory"),
   N_("directory from which templates will be used")),
-   OPT_STRING_LIST(0, "reference", _reference, N_("repo"),
+   OPT_STRING_LIST(0, "reference", _required_reference, N_("repo"),
N_("reference repository")),
OPT_BOOL(0, "dissociate", _dissociate,
 N_("use --reference only while cloning")),
@@ -300,7 +300,7 @@ static int add_one_reference(struct string_list_item *item, 
void *cb_data)
 
 static void setup_reference(void)
 {
-   for_each_string_list(_reference, add_one_reference, NULL);
+   for_each_string_list(_required_reference, add_one_reference, 
NULL);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -952,7 +952,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
git_config_set(key.buf, repo);
strbuf_reset();
 
-   if (option_reference.nr)
+   if (option_required_reference.nr)
setup_reference();
 
fetch_pattern = value.buf;
-- 
2.9.2.730.g525ad04.dirty

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

[PATCHv5 7/8] clone: implement optional references

[Stefan Beller]

In a later patch we want to try to create alternates for submodules,
but they might not exist in the referenced superproject. So add a way
to skip the non existing references and report them.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  5 -
 builtin/clone.c | 35 +--
 2 files changed, 29 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index ec41d3d..e316c4b 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -90,13 +90,16 @@ If you want to break the dependency of a repository cloned 
with `-s` on
 its source repository, you can simply run `git repack -a` to copy all
 objects from the source repository into a pack in the cloned repository.
 
---reference ::
+--reference[-if-able] ::
If the reference repository is on the local machine,
automatically setup `.git/objects/info/alternates` to
obtain objects from the reference repository.  Using
an already existing repository as an alternate will
require fewer objects to be copied from the repository
being cloned, reducing network and local storage costs.
+   When using the `--reference-if-able`, a non existing
+   directory is skipped with a warning instead of aborting
+   the clone.
 +
 *NOTE*: see the NOTE for the `--shared` option, and also the
 `--dissociate` option.
diff --git a/builtin/clone.c b/builtin/clone.c
index ef5db7c..0182665 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,6 +51,7 @@ static int option_progress = -1;
 static enum transport_family family;
 static struct string_list option_config = STRING_LIST_INIT_NODUP;
 static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
+static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
 
@@ -81,6 +82,8 @@ static struct option builtin_clone_options[] = {
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", _required_reference, N_("repo"),
N_("reference repository")),
+   OPT_STRING_LIST(0, "reference-if-able", _optional_reference,
+   N_("repo"), N_("reference repository")),
OPT_BOOL(0, "dissociate", _dissociate,
 N_("use --reference only while cloning")),
OPT_STRING('o', "origin", _origin, N_("name"),
@@ -283,24 +286,36 @@ static void strip_trailing_slashes(char *dir)
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
struct strbuf err = STRBUF_INIT;
-   struct strbuf sb = STRBUF_INIT;
+   int *required = cb_data;
char *ref_git = compute_alternate_path(item->string, );
 
-   if (!ref_git)
-   die("%s", err.buf);
-
-   strbuf_addf(, "%s/objects", ref_git);
-   add_to_alternates_file(sb.buf);
+   if (!ref_git) {
+   if (*required)
+   die("%s", err.buf);
+   else
+   fprintf(stderr,
+   _("info: Could not add alternate for '%s': 
%s\n"),
+   item->string, err.buf);
+   } else {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addf(, "%s/objects", ref_git);
+   add_to_alternates_file(sb.buf);
+   strbuf_release();
+   }
 
-   free(ref_git);
strbuf_release();
-   strbuf_release();
+   free(ref_git);
return 0;
 }
 
 static void setup_reference(void)
 {
-   for_each_string_list(_required_reference, add_one_reference, 
NULL);
+   int required = 1;
+   for_each_string_list(_required_reference,
+add_one_reference, );
+   required = 0;
+   for_each_string_list(_optional_reference,
+add_one_reference, );
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -952,7 +967,7 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
git_config_set(key.buf, repo);
strbuf_reset();
 
-   if (option_required_reference.nr)
+   if (option_required_reference.nr || option_optional_reference.nr)
setup_reference();
 
fetch_pattern = value.buf;
-- 
2.9.2.730.g525ad04.dirty

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

[PATCHv5 4/8] submodule--helper update-clone: allow multiple references

[Stefan Beller]

Allow the user to pass in multiple references to update_clone.
Currently this is only internal API, but once the shell script is
replaced by a C version, this is needed.

This fixes an API bug between the shell script and the helper.
Currently the helper accepts "--reference" "--reference=foo"
as a OPT_STRING whose value happens to be "--reference=foo", and
then uses

if (suc->reference)
argv_array_push(>args, suc->reference)

where suc->reference _is_ "--reference=foo" when invoking the
underlying "git clone", it cancels out.

With this change we omit one of the "--reference" arguments when
passing references from the shell script to the helper.

Signed-off-by: Stefan Beller 
---
 builtin/submodule--helper.c | 14 +-
 git-submodule.sh|  2 +-
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ce9b3e9..7096848 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -584,7 +584,7 @@ struct submodule_update_clone {
/* configuration parameters which are passed on to the children */
int quiet;
int recommend_shallow;
-   const char *reference;
+   struct string_list references;
const char *depth;
const char *recursive_prefix;
const char *prefix;
@@ -600,7 +600,8 @@ struct submodule_update_clone {
int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, NULL, NULL, NULL, NULL, \
+   SUBMODULE_UPDATE_STRATEGY_INIT, 0, -1, STRING_LIST_INIT_DUP, \
+   NULL, NULL, NULL, \
STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 
 
@@ -710,8 +711,11 @@ static int prepare_to_clone_next_submodule(const struct 
cache_entry *ce,
argv_array_pushl(>args, "--path", sub->path, NULL);
argv_array_pushl(>args, "--name", sub->name, NULL);
argv_array_pushl(>args, "--url", url, NULL);
-   if (suc->reference)
-   argv_array_push(>args, suc->reference);
+   if (suc->references.nr) {
+   struct string_list_item *item;
+   for_each_string_list_item(item, >references)
+   argv_array_pushl(>args, "--reference", 
item->string, NULL);
+   }
if (suc->depth)
argv_array_push(>args, suc->depth);
 
@@ -830,7 +834,7 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "update", ,
   N_("string"),
   N_("rebase, merge, checkout or none")),
-   OPT_STRING(0, "reference", , N_("repo"),
+   OPT_STRING_LIST(0, "reference", , N_("repo"),
   N_("reference repository")),
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
diff --git a/git-submodule.sh b/git-submodule.sh
index c90dc33..3b412f5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -575,7 +575,7 @@ cmd_update()
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
-   ${reference:+--reference "$reference"} \
+   ${reference:+"$reference"} \
${depth:+--depth "$depth"} \
${recommend_shallow:+"$recommend_shallow"} \
${jobs:+$jobs} \
-- 
2.9.2.730.g525ad04.dirty

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

[PATCHv5 0/8] git clone: Marry --recursive and --reference

[Stefan Beller]

v5:

Thanks Junio, Ramsay for comments.

As the comments only address programming work as opposed to design, the 
interdiff is rather small:

diff --git a/builtin/clone.c b/builtin/clone.c
index 0593aee..404c5e8 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -285,23 +285,26 @@ static void strip_trailing_slashes(char *dir)
 
 static int add_one_reference(struct string_list_item *item, void *cb_data)
 {
-   struct strbuf sb = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
int *required = cb_data;
-   char *ref_git = compute_alternate_path(item->string, );
+   char *ref_git = compute_alternate_path(item->string, );
 
if (!ref_git) {
if (*required)
-   die("%s", sb.buf);
+   die("%s", err.buf);
else
fprintf(stderr,
_("info: Could not add alternate for '%s': 
%s\n"),
-   item->string, sb.buf);
+   item->string, err.buf);
} else {
+   struct strbuf sb = STRBUF_INIT;
strbuf_addf(, "%s/objects", ref_git);
add_to_alternates_file(sb.buf);
+   strbuf_release();
}
 
-   strbuf_release();
+   strbuf_release();
+   free(ref_git);
return 0;
 }
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 472b1d9..681b91d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -484,7 +484,7 @@ struct submodule_alternate_setup {
 #define SUBMODULE_ALTERNATE_SETUP_INIT { NULL, \
SUBMODULE_ALTERNATE_ERROR_IGNORE, NULL }
 
-int add_possible_reference_from_superproject(
+static int add_possible_reference_from_superproject(
struct alternate_object_database *alt, void *sas_cb)
 {
struct submodule_alternate_setup *sas = sas_cb;
diff --git a/sha1_file.c b/sha1_file.c
index 2489653..0fe5aa3 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -435,11 +435,12 @@ char *compute_alternate_path(const char *path, struct 
strbuf *err)
 {
char *ref_git = NULL;
const char *repo, *ref_git_s;
-   struct strbuf err_buf = STRBUF_INIT;
+   int seen_error = 0;
 
ref_git_s = real_path_if_valid(path);
if (!ref_git_s) {
-   strbuf_addf(_buf, _("path '%s' does not exist"), path);
+   seen_error = 1;
+   strbuf_addf(err, _("path '%s' does not exist"), path);
goto out;
} else
/*
@@ -462,40 +463,41 @@ char *compute_alternate_path(const char *path, struct 
strbuf *err)
ref_git = ref_git_git;
} else if (!is_directory(mkpath("%s/objects", ref_git))) {
struct strbuf sb = STRBUF_INIT;
+   seen_error = 1;
if (get_common_dir(, ref_git)) {
-   strbuf_addf(_buf,
+   strbuf_addf(err,
_("reference repository '%s' as a linked "
  "checkout is not supported yet."),
path);
goto out;
}
 
-   strbuf_addf(_buf, _("reference repository '%s' is not a "
+   strbuf_addf(err, _("reference repository '%s' is not a "
"local repository."), path);
goto out;
}
 
if (!access(mkpath("%s/shallow", ref_git), F_OK)) {
-   strbuf_addf(_buf, _("reference repository '%s' is shallow"),
+   strbuf_addf(err, _("reference repository '%s' is shallow"),
path);
+   seen_error = 1;
goto out;
}
 
if (!access(mkpath("%s/info/grafts", ref_git), F_OK)) {
-   strbuf_addf(_buf,
+   strbuf_addf(err,
_("reference repository '%s' is grafted"),
path);
+   seen_error = 1;
goto out;
}
 
 out:
-   if (err_buf.len) {
-   strbuf_addbuf(err, _buf);
+   if (seen_error) {
free(ref_git);
ref_git = NULL;
}
 
-   strbuf_release(_buf);
return ref_git;
 }
 
Thanks,
Stefan


v4:
Thanks to Junios critial questions regarding the design, I took a step back
to look at the bigger picture, again.

new patches:
  clone: factor out checking for an alternate path
  clone: recursive and reference option triggers submodule alternates
  
The last patch redesigns completely how we approach the problem.
Now there are no new command line options (that relate to the problem
of marrying --recursive and --reference), but instead we communicate
everything via configuration options to have a lasting effect (i.e.
submodule update remembers the decision of the initial setup)

Thanks,
Stefan

v3:

Thanks to Junios critial 

[PATCHv5 2/8] t7408: merge short tests, factor out testing method

[Stefan Beller]

Tests consisting of one line each can be consolidated to have fewer tests
to run as well as fewer lines of code.

When having just a few git commands, do not create a new shell but
use the -C flag in Git to execute in the correct directory.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 48 ++
 1 file changed, 25 insertions(+), 23 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index b84c6748..dff47af 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,6 +8,15 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
+test_alternate_is_used () {
+   alternates_file="$1" &&
+   working_dir="$2" &&
+   test_line_count = 1 "$alternates_file" &&
+   echo "0 objects, 0 kilobytes" >expect &&
+   git -C "$working_dir" count-objects >actual &&
+   test_cmp expect actual
+}
+
 test_expect_success 'preparing first repository' '
test_create_repo A &&
(
@@ -40,16 +49,14 @@ test_expect_success 'preparing superproject' '
)
 '
 
-test_expect_success 'submodule add --reference' '
+test_expect_success 'submodule add --reference uses alternates' '
(
cd super &&
git submodule add --reference ../B "file://$base_dir/A" sub &&
-   git commit -m B-super-added
-   )
-'
-
-test_expect_success 'after add: existence of info/alternates' '
-   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+   git commit -m B-super-added &&
+   git repack -ad
+   ) &&
+   test_alternate_is_used super/.git/modules/sub/objects/info/alternates 
super/sub
 '
 
 test_expect_success 'that reference gets used with add' '
@@ -61,23 +68,18 @@ test_expect_success 'that reference gets used with add' '
)
 '
 
-test_expect_success 'cloning superproject' '
-   git clone super super-clone
-'
-
-test_expect_success 'update with reference' '
-   cd super-clone && git submodule update --init --reference ../B
-'
-
-test_expect_success 'after update: existence of info/alternates' '
-   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
-'
+# The tests up to this point, and repositories created by them
+# (A, B, super and super/sub), are about setting up the stage
+# for subsequent tests and meant to be kept throughout the
+# remainder of the test.
+# Tests from here on, if they create their own test repository,
+# are expected to clean after themselves.
 
-test_expect_success 'that reference gets used with update' '
-   cd super-clone/sub &&
-   echo "0 objects, 0 kilobytes" >expected &&
-   git count-objects >current &&
-   diff expected current
+test_expect_success 'updating superproject keeps alternates' '
+   test_when_finished "rm -rf super-clone" &&
+   git clone super super-clone &&
+   git -C super-clone submodule update --init --reference ../B &&
+   test_alternate_is_used 
super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
 test_done
-- 
2.9.2.730.g525ad04.dirty

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

[PATCHv5 1/8] t7408: modernize style

[Stefan Beller]

No functional change intended. This commit only changes formatting
to the style we recently use, e.g. starting the body of a test with a
single quote on the same line as the header, and then having the test
indented in the following lines.

Whenever we change directories, we do that in subshells.

Signed-off-by: Stefan Beller 
---
 t/t7408-submodule-reference.sh | 140 +
 1 file changed, 71 insertions(+), 69 deletions(-)

diff --git a/t/t7408-submodule-reference.sh b/t/t7408-submodule-reference.sh
index eaea19b..b84c6748 100755
--- a/t/t7408-submodule-reference.sh
+++ b/t/t7408-submodule-reference.sh
@@ -8,74 +8,76 @@ test_description='test clone --reference'
 
 base_dir=$(pwd)
 
-U=$base_dir/UPLOAD_LOG
-
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo first > file1 &&
-git add file1 &&
-git commit -m A-initial'
-
-cd "$base_dir"
-
-test_expect_success 'preparing second repository' \
-'git clone A B && cd B &&
-echo second > file2 &&
-git add file2 &&
-git commit -m B-addition &&
-git repack -a -d &&
-git prune'
-
-cd "$base_dir"
-
-test_expect_success 'preparing superproject' \
-'test_create_repo super && cd super &&
-echo file > file &&
-git add file &&
-git commit -m B-super-initial'
-
-cd "$base_dir"
-
-test_expect_success 'submodule add --reference' \
-'cd super && git submodule add --reference ../B "file://$base_dir/A" sub &&
-git commit -m B-super-added'
-
-cd "$base_dir"
-
-test_expect_success 'after add: existence of info/alternates' \
-'test_line_count = 1 super/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with add' \
-'cd super/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
-
-test_expect_success 'cloning superproject' \
-'git clone super super-clone'
-
-cd "$base_dir"
-
-test_expect_success 'update with reference' \
-'cd super-clone && git submodule update --init --reference ../B'
-
-cd "$base_dir"
-
-test_expect_success 'after update: existence of info/alternates' \
-'test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used with update' \
-'cd super-clone/sub &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-diff expected current'
-
-cd "$base_dir"
+test_expect_success 'preparing first repository' '
+   test_create_repo A &&
+   (
+   cd A &&
+   echo first >file1 &&
+   git add file1 &&
+   git commit -m A-initial
+   )
+'
+
+test_expect_success 'preparing second repository' '
+   git clone A B &&
+   (
+   cd B &&
+   echo second >file2 &&
+   git add file2 &&
+   git commit -m B-addition &&
+   git repack -a -d &&
+   git prune
+   )
+'
+
+test_expect_success 'preparing superproject' '
+   test_create_repo super &&
+   (
+   cd super &&
+   echo file >file &&
+   git add file &&
+   git commit -m B-super-initial
+   )
+'
+
+test_expect_success 'submodule add --reference' '
+   (
+   cd super &&
+   git submodule add --reference ../B "file://$base_dir/A" sub &&
+   git commit -m B-super-added
+   )
+'
+
+test_expect_success 'after add: existence of info/alternates' '
+   test_line_count = 1 super/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with add' '
+   (
+   cd super/sub &&
+   echo "0 objects, 0 kilobytes" >expected &&
+   git count-objects >current &&
+   diff expected current
+   )
+'
+
+test_expect_success 'cloning superproject' '
+   git clone super super-clone
+'
+
+test_expect_success 'update with reference' '
+   cd super-clone && git submodule update --init --reference ../B
+'
+
+test_expect_success 'after update: existence of info/alternates' '
+   test_line_count = 1 super-clone/.git/modules/sub/objects/info/alternates
+'
+
+test_expect_success 'that reference gets used with update' '
+   cd super-clone/sub &&
+   echo "0 objects, 0 kilobytes" >expected &&
+   git count-objects >current &&
+   diff expected current
+'
 
 test_done
-- 
2.9.2.730.g525ad04.dirty

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

Re: [PATCHv4 5/8] clone: factor out checking for an alternate path

[Stefan Beller]

On Mon, Aug 15, 2016 at 1:36 PM, Junio C Hamano  wrote:
>
>>> Why do you need "err_buf", instead of directly writing the error to
>>> "err", especially if "err" is not optional?
>>>
 + ...
 +out:
 + if (err_buf.len) {
>>
>> If we were directly writing to err, we would have checked
>> err.len here. Then you open up a subtle way of saying "dry run"
>> by giving a non empty error buffer.
>>
>> I contemplated doing that actually instead of splitting up into 2 functions,
>> but I considered that bad taste as it would require documentation.
>
> Sorry, but I do not understand this response at all.

Me neither. I think I elided the interesting part.

> I am still
> talking about keeping one function "compute_alternate_path()".  The
> suggestion was just to drop "struct strbuf err_buf" local variable,
> and instead add the error messages the patch adds to err_buf with
> code like:
>
>> + if (!ref_git_s) {
>> + strbuf_addf(_buf, _("path '%s' does not exist"), path);
>> + goto out;
>
> to directly do
>
> strbuf_addf(err, _("path '%s' does not exist"), path);

done.

> err->len at "out:" label, or (2) using a new bool "seen_error = 0"
> and setting it to true when you diagnose an error.  The latter would
> make the code a bit more verbose but I suspect the result would be
> easier to read than the former.
>

(2) is very readable, will reroll with that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] difftool: always honor "command not found" exit code

[John Keeping]

On Mon, Aug 15, 2016 at 01:21:22PM -0700, Junio C Hamano wrote:
> "Tom Tanner (BLOOMBERG/ LONDON)"  writes:
> 
> > From: gits...@pobox.com
> > To: j...@keeping.me.uk
> > Cc: Tom Tanner (BLOOMBERG/ LONDON), dav...@gmail.com, git@vger.kernel.org
> > At: 08/14/16 04:21:18
> >
> > John Keeping  writes:
> > ...
> >> POSIX specifies 127 as the exit status for "command not found" and 126
> >> for "command found but is not executable" [1] and at least bash and dash
> >> follow this specification, while diff utilities generally use "1" for
> >> the exit status we want to ignore.
> >>
> >> Handle 126 and 127 as special values, assuming that they always mean
> >> that the command could not be executed.
> >
> > Sounds like a reasonable thing to do.  Will queue; thanks.
> 
> > Would it be possible to also treat signals (128 and above) as
> > 'special' values as well (as I've seen some merge tools self
> > destruct like that from time to time)
> 
> Certainly, it feels safer to notice an unusual exit status code and
> error out to force the user to take notice, but that reasoning
> assumes that "128 and above" are noteworthy exceptions.

Reading further in POSIX:

The exit status of a command that terminated because it received
a signal shall be reported as greater than 128.

I think if we accept the argument above about diff utilities generally
using low numbers for the status values we're ignoring intentionally,
then we can just treat any value above 125 as a fatal error.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

On Mon, Aug 15, 2016 at 2:10 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Jeff King  writes:
>>
>>> I don't think doing it this way is _wrong_. It just feels sort of
>>> pointlessly over-engineered. It's also a little weird that all of the:
>>>
>>>   if (advice_foo)
>>>
>>> will trigger because "advice_foo" is set to -1. I think it does the
>>> right thing, but it feels like a bug (the value is now a tri-state, and
>>> we silently collapse two states into one).
>>
>> Guilty as charged.  I do agree that this is over-engineered.
>
> Let's discard 1/2 and amend 2/2 with this incremental.

That is fine with me.

Thanks,
Stefan

>
>
>
>  builtin/checkout.c | 3 +--
>  t/t2020-checkout-detach.sh | 5 -
>  2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2a32b5f..337c35a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct 
> checkout_opts *opts,
>REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
> if (!opts->quiet) {
> if (old->path &&
> -   (advice_detached_head == 1 ||
> -(advice_detached_head == -1 && 
> !opts->force_detach)))
> +   advice_detached_head == 1 && !opts->force_detach)
> detach_advice(new->name);
> describe_detached_head(_("HEAD is now at"), 
> new->commit);
> }
> diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
> index fe311a1..fbb4ee9 100755
> --- a/t/t2020-checkout-detach.sh
> +++ b/t/t2020-checkout-detach.sh
> @@ -180,11 +180,6 @@ test_expect_success 'no advice given for explicit 
> detached head state' '
> git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
> test_cmp expect.no-advice actual &&
>
> -   # explicitly ask advice
> -   test_config advice.detachedHead true &&
> -   git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
> -   test_cmp expect.advice actual &&
> -
> # explicitly decline advice
> test_config advice.detachedHead false &&
> git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 02:10:36PM -0700, Junio C Hamano wrote:

> > Guilty as charged.  I do agree that this is over-engineered.
> 
> Let's discard 1/2 and amend 2/2 with this incremental.

Looks good (though I really am OK the other way if people feel
strongly).

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2a32b5f..337c35a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct 
> checkout_opts *opts,
>  REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
>   if (!opts->quiet) {
>   if (old->path &&
> - (advice_detached_head == 1 ||
> -  (advice_detached_head == -1 && 
> !opts->force_detach)))
> + advice_detached_head == 1 && !opts->force_detach)

Maybe s/== 1//?

That's more idiomatic, though maybe you were future-proofing in case we
turn it into a tri-state later. :)

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

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> I don't think doing it this way is _wrong_. It just feels sort of
>> pointlessly over-engineered. It's also a little weird that all of the:
>>
>>   if (advice_foo)
>>
>> will trigger because "advice_foo" is set to -1. I think it does the
>> right thing, but it feels like a bug (the value is now a tri-state, and
>> we silently collapse two states into one).
>
> Guilty as charged.  I do agree that this is over-engineered.

Let's discard 1/2 and amend 2/2 with this incremental.



 builtin/checkout.c | 3 +--
 t/t2020-checkout-detach.sh | 5 -
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a32b5f..337c35a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
if (!opts->quiet) {
if (old->path &&
-   (advice_detached_head == 1 ||
-(advice_detached_head == -1 && 
!opts->force_detach)))
+   advice_detached_head == 1 && !opts->force_detach)
detach_advice(new->name);
describe_detached_head(_("HEAD is now at"), 
new->commit);
}
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index fe311a1..fbb4ee9 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -180,11 +180,6 @@ test_expect_success 'no advice given for explicit detached 
head state' '
git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
test_cmp expect.no-advice actual &&
 
-   # explicitly ask advice
-   test_config advice.detachedHead true &&
-   git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
-   test_cmp expect.advice actual &&
-
# explicitly decline advice
test_config advice.detachedHead false &&
git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Jeff King  writes:

> I don't think doing it this way is _wrong_. It just feels sort of
> pointlessly over-engineered. It's also a little weird that all of the:
>
>   if (advice_foo)
>
> will trigger because "advice_foo" is set to -1. I think it does the
> right thing, but it feels like a bug (the value is now a tri-state, and
> we silently collapse two states into one).

Guilty as charged.  I do agree that this is over-engineered.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano  writes:

> Good thinking.
>
> Some tests may have to be skipped on platforms that cannot express
> certain paths, but even then they shouldn't ship a file with
> pathname that cannot even be checked out (they should instead create
> and use such a path, protected behind filesystem specific test
> prerequisite).
>
>> +test-lint-filenames:
>> +@illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
>
> This pattern must exclude questionables on either NTFS or HFS+; it
> is ironic that it is not even sufficient to limit ourselves to the
> Portable Character Set [*1*], but such is life.
>
> By the way, doesn't ls-files take pathspec glob, saving one extra
> process to run grep?
>
> master$ git ls-files '*["*:<>?\\|]*'
> pu$ git ls-files '*["*:<>?\\|]*'
> t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side

One more thing you may want to exclude is HT.  Here is a suggested
reroll.  I reworded to avoid a subjective "truly platform-independent",
which is not what we intend to aim for anyway (we only try to support
the platforms we care about).

-- >8 --
From: Johannes Schindelin 
Date: Mon, 15 Aug 2016 16:08:41 +0200
Subject: [PATCH] t/Makefile: make sure that paths can be checked out on
 platforms we care

Some pathnames that are okay on ext4 and on HFS+ cannot be checked
out on Windows.  Tests that want to see operations on such paths on
filesystems that support them must do so behind appropriate test
prerequisites, and must not include them in the source tree (instead
they should create them when they run).  Otherwise, the source tree
cannot be even checked out.

Make sure that double-quotes, asterisk, colon, greater/less-than,
question-mark, backslash, tab and vertical-bar never appears in the
pathnames with a new test-lint-* target as part of a `make test`.

Noticed when a topic wanted to add a pathname with '>' in it.  A
check like this will prevent a similar problems from happening.

Signed-off-by: Johannes Schindelin 
Signed-off-by: Junio C Hamano 
---
 t/Makefile | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..d4b2a50 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+   test-lint-filenames
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+   @bad=$$(git ls-files '*["*:<>?\\|]*'); \
+   test -z "$$bad" || { \
+   echo >&2 "do not use non-portable file name(s): $$bad"; exit 1; 
}
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.10.0-rc0-132-gce76bc9

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

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Jeff King  writes:

>> So a comment outside [section "name"] is tricky; it needs some
>> mechanism (or convention) to tell us if it is about the particular
>> section, or it is about the location in the configuration file.
>
> Keep in mind that even "outside" is hard, because sections do not
> explicitly close.

Yes, that was what I wanted to say.  We'd be deep in the
"convention" territory.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Philip Oakley]

From: "Jacob Keller" 
[nip]


I've no problem with more extensive methods for those preparing very big
patch series, or with those needing to merge together a lot of series and
want to keep the cover letters, but ensuring that a simple flow is 
possible

should still be there.
--
Philip



Some people have suggested this simple idea, and I like it, but they
did mention that modifying the cover letter now requires a rebase over
a potentially large series of patches, which can get annoying.

Thanks,
Jake


They can just add "squash! cover! " commits for that ;-) Though more 
likely the advanced workflow would be used... We'll need both (more than 
one) options.

--
Philip 


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

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Philip Oakley]

From: "Junio C Hamano" 

"Philip Oakley"  writes:


I'm still not sure this is enough. One of the problems back when I
introduced the --guides option (65f9835 (builtin/help.c: add --guide
option, 2013-04-02)) was that we had no easy way of determining what
guides were available, especially given the *nix/Windows split where
the help defaults are different (--man/--html).

At the time[1] we (I) punted on trying to determine which guides were
actually installed, and just created a short list of the important
guides, which I believe you now check. However the less common guides
are still there (gitcvs-migration?), and others may be added locally.


I think we should do both; "git help cvs-migration" should keep the
same codeflow and behaviour as we have today (so that it would still
work), while "git cvs-migration --help" should say "'cvs-migration'
is not a git command".  That would be a good clean-up anyway.

It obviously cannot be done if git.c::handle_builtin() does the same
"swap  --help to help " hack, but we could improve that
part (e.g. rewrite it to "help --swapped " to allow cmd_help()
to notice).  When the user said " --help", we don't do guides,
when we swapped the word order, we check with guides, too.

The other option is to simply build a guide-list in exactly the same format 
as the command list (which if it works can be merged later). Re-use the 
existing code, etc.


I did propose that in my very first patch series, but it was probably a step 
too far at the time, as it stepped on the toes of your (junio's) script for 
the command list.


The link in my previous patch got mangled a possible start point for Ralf 
for looking at building a guide list would be 
http://public-inbox.org/git/1361660761-1932-7-git-send-email-philipoak...@iee.org/ 
(which worked back then ;-)


Philip


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

Re: [RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Jeff King]

On Mon, Aug 15, 2016 at 09:57:29PM +0200, Christian Couder wrote:

> From: Jeff King 
> 
> Receive-pack feeds its input to either index-pack or
> unpack-objects, which will happily accept as many bytes as
> a sender is willing to provide. Let's allow an arbitrary
> cutoff point where we will stop writing bytes to disk.
> 
> What has already been written to disk can be cleaned
> outside of receive-pack.

This second paragraph hints at a related problem.

Git is generally happy to leave tmp_pack_* around to be cleaned up later
next time git-gc runs. Including its default 2-week grace time.

So imagine that tries to "git push" in a loop. And each time they push,
you say "nope, that's too big". And each time you acquire a new 2GB
tmp_pack file. If your goal was to prevent somebody from streaming
straight to your filesystem and filling up your disk, then it wasn't
very successful. :)

The simple fix is to call register_tempfile() in open_pack_file(), and
just have index-pack clean up the file on its way out.

But there are harder cases. For instance, imagine somebody pushes a
500MB file, and you have a pre-receive hook that says "too big; I won't
accept this". And then they push in a loop, as before. You've accepted
the incoming pack into the repository by the time the pre-receive runs.
You can't just delete it, because you don't know if other simultaneous
processes have started to depend on the objects.

To solve that, I have patches that put incoming packfiles into a
"quarantine" area, then run the connectivity check and pre-receive hooks
with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
then we either move the quarantine packs into the real repo, or blow
away the tmpdir, depending on whether the hooks said the objects were
OK.

Those are patches I plan to share upstream but just haven't gotten
around to yet.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 92e1213..7627f7f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;
>  static int advertise_atomic_push = 1;
>  static int advertise_push_options;
>  static int unpack_limit = 100;
> +static off_t max_input_size;
>  static int report_status;
>  static int use_sideband;
>  static int use_atomic;
> @@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const 
> char *value, void *cb)
>   return 0;
>   }
>  
> + if (strcmp(var, "receive.maxsize") == 0) {
> + max_input_size = git_config_ulong(var, value);
> + return 0;
> + }

Another off_t/ulong mismatch. I think you want git_config_int64() here.

> @@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct 
> shallow_info *si)
>   if (fsck_objects)
>   argv_array_pushf(, "--strict%s",
>   fsck_msg_types.buf);
> + if (max_input_size)
> + argv_array_pushf(, "--max-input-size=%lu",
> + max_input_size);

And here, PRIuMAX and uintmax_t. Or perhaps simpler, just store the
value as a string here and pass it on to index-pack (which would then
need to learn to handle suffixes like "2g"). We do a similar trick in
repack; see b861e23 (repack: propagate pack-objects options as strings,
2014-01-22).

> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
> new file mode 100755
> index 000..d3a4d1a
> --- /dev/null
> +++ b/t/t5546-push-limits.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +test_description='check input limits for pushing'
> +. ./test-lib.sh
> +
> +test_expect_success 'create known-size commit' '
> + test-genrandom foo 1024 >file &&
> + git add file &&
> + test_commit one-k
> +'
> +
> +test_expect_success 'create remote repository' '
> + git init --bare dest &&
> + git --git-dir=dest config receive.unpacklimit 1
> +'

We're going to do basically the same battery of tests against an
unpacklimit of "1" (to catch index-pack) and of "10" (to catch
unpack-objects). It might be clearer to just have a for-loop like:

  for unpacklimit in 1 100
  do
test_expect_success 'create remote repository' '
rm -rf dest &&
git init --bare dest &&
git -C dest config receive.unpacklimit $unpacklimit
'

test_expect_success 'receive.maxsize rejects push' '
git -C dest config receive.maxsize 512 &&
test_must_fail git push dest HEAD &&
'

test_expect_success 'bumping limit allows push' '
git -C dest config receive.maxsize 4k &&
git push dest HEAD
'
  done

and it's probably worth a comment at the top of the loop explaining what
the heck those numbers mean. :)

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

Re: storing cover letter of a patch series?

[Junio C Hamano]

Jacob Keller  writes:

> Some people have suggested this simple idea, and I like it, but they
> did mention that modifying the cover letter now requires a rebase over
> a potentially large series of patches, which can get annoying.

That can be simply solved by keeping the cover at the end.  When you
are updating the real patch on the series with "rebase -i", you
would have a chance to update the cover at the same time that way.

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

Re: [PATCHv4 5/8] clone: factor out checking for an alternate path

[Junio C Hamano]

Stefan Beller  writes:

> On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
>>> + struct strbuf sb = STRBUF_INIT;
>>> + char *ref_git = compute_alternate_path(item->string, );
>>
>> Who owns the memory for ref_git?
>
> The caller of compute_alternate_path(..), which makes
> add_one_reference faulty as of this patch.
>
>>
>>> - if (!access(mkpath("%s/shallow", ref_git), F_OK))
>>> - die(_("reference repository '%s' is shallow"), item->string);
>>> + if (!ref_git)
>>> + die("%s", sb.buf);
>>
>> Presumably the second argument to compute_alternate_path() is a
>> strbuf to receive the error message?  It is unfortunate that the
>> variable used for this purpose is a bland "sb", but perhaps that
>> cannot be helped as you would reuse that strbuf for a different
>> purpose (i.e. not to store the error message, but to formulate a
>> pathname).
>
> Ok. I had an intermediate version with 2 strbufs but for some reason I
> decided one is better. We'll have 2 again. (err and sb; sb will have a
> smaller scope only in the else part.)

My "unfortunate" was meant to say "it is unfortunate that this is
the best, adding one extra variable is not worth the cost".

>> Why do you need "err_buf", instead of directly writing the error to
>> "err", especially if "err" is not optional?
>>
>>> + ...
>>> +out:
>>> + if (err_buf.len) {
>
> If we were directly writing to err, we would have checked
> err.len here. Then you open up a subtle way of saying "dry run"
> by giving a non empty error buffer.
>
> I contemplated doing that actually instead of splitting up into 2 functions,
> but I considered that bad taste as it would require documentation.

Sorry, but I do not understand this response at all.  I am still
talking about keeping one function "compute_alternate_path()".  The
suggestion was just to drop "struct strbuf err_buf" local variable,
and instead add the error messages the patch adds to err_buf with
code like:

> + if (!ref_git_s) {
> + strbuf_addf(_buf, _("path '%s' does not exist"), path);
> + goto out;

to directly do

strbuf_addf(err, _("path '%s' does not exist"), path);

instead.  That way you do not have to move the bytes around from one
buffer to the other, like this:

>>> + strbuf_addbuf(err, _buf);
>>> + free(ref_git);
>>> + ref_git = NULL;
>>> + }

If you allow err->len to be non-zero upon entry, you'd need a way to
remember if you noticed an error, so that you can free and clear
ref_git after "out:" label, and doing so with a separate variable
would make the code more readable, I would think, either by (1)
recording the original err->len upon entry, and comparing it with
err->len at "out:" label, or (2) using a new bool "seen_error = 0"
and setting it to true when you diagnose an error.  The latter would
make the code a bit more verbose but I suspect the result would be
easier to read than the former.

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

Re: [PATCH 8/8] diff: improve positioning of add/delete blocks in diffs

[Junio C Hamano]

Stefan Beller  writes:

> Maybe we can enable Michaels heuristic with the same
> config/command line flag, i.e. "the flag changes its algorithm"?

I think that is a very sensible proposal.  After all, the name
diff.compactionHeuristic only tells us what part of the diff process
the heuristic is used, and does not say anything about what the
heuristics does.  It is neutral between "take a blank as a hint" vs
"take indentation leveles as a hint".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GSOC Update] Week 15

[Pranit Bauva]

From: Pranit Bauva 
Subject: [GSOC Update] Week 15

=== SUMMARY ==
My public git.git is available here[1]. I regularly keep pushing my work so
anyone interested can track me there. Feel free to participate in the
discussions going on PRs with my mentors. Your comments are valuable.


== INTRODUCTION  ==
The purpose of this project is to convert the git-bisect utility which partly
exists in the form of shell scripts to C code so as to make it more portable.
I plan to do this by converting each function to C and then calling it from
git-bisect.sh so as to use the existing test suite to test the function which
is converted.

Mentors:
Christian Couder 
Lars Schneider 


 Updates ===
Things which were done in this week:

 * I have converted bisect_start() and the bug has been eliminated too.

 * I have converted bisect_next() but still has some bugs. A notable
   problem is that the bisect.c code uses exit() statements and `trap` in
   the shell code for cleanup with `bisect_clean_state()`. So as
   suggested by Lars, I intend to use `bisect_clean_state()` along with
   die() as cleanup (or goto preferably).

= NEXT STEPS =
Things which would be done in the coming week:

 * Resend all patches rebased wtih v2.10-rc0.

 * bisect_next()

 * Following that I will convert bisect_auto_start()

 * Then bisect_replay().

=== My Patches (GSoC project only) ===

 * My current work is sent out to the mailing list here[2] which contains
   the whole conversion. Please don't merge the previous patches to next
   yet. Junio has requested me to rebase it on v2.10-rc0 so will resend
   it. Plus there are some change in the patch 04/13 which change the
   location of the function `bisect_clean_state()` from
   builtin/bisect--helper.c to bisect.c .

[1]: https://github.com/pranitbauva1997/git
[2]: 
https://public-inbox.org/git/010201567675adc1-17e27495-6b36-40d1-836d-814da029fcc4-000...@eu-west-1.amazonses.com/

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

Re: [PATCH] difftool: always honor "command not found" exit code

[Junio C Hamano]

"Tom Tanner (BLOOMBERG/ LONDON)"  writes:

> From: gits...@pobox.com
> To: j...@keeping.me.uk
> Cc: Tom Tanner (BLOOMBERG/ LONDON), dav...@gmail.com, git@vger.kernel.org
> At: 08/14/16 04:21:18
>
> John Keeping  writes:
> ...
>> POSIX specifies 127 as the exit status for "command not found" and 126
>> for "command found but is not executable" [1] and at least bash and dash
>> follow this specification, while diff utilities generally use "1" for
>> the exit status we want to ignore.
>>
>> Handle 126 and 127 as special values, assuming that they always mean
>> that the command could not be executed.
>
> Sounds like a reasonable thing to do.  Will queue; thanks.

> Would it be possible to also treat signals (128 and above) as
> 'special' values as well (as I've seen some merge tools self
> destruct like that from time to time)

Certainly, it feels safer to notice an unusual exit status code and
error out to force the user to take notice, but that reasoning
assumes that "128 and above" are noteworthy exceptions.

I do not have a strong opinion on that part.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-mergetool reverse file ordering

[Luis Gutierrez]

> Thoughts?  Would you be interested in helping work up a patch
> for this idea?  At a minimum we should also write a test case in
> t/t7610-mergetool.sh to verify that it works as advertised.

> Why not reuse the existing diff.orderFile config variable?  (Also
> supported by the -O option to git-diff).


I'll be happy to write a testcase, and to re-use the -O
(diff.orderFile config var) option to git-diff as sugguested by John
Keeping.

Is this the final spec?



I'll be happy to do that.

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

Re: [RFC/PATCH 2/3] unpack-objects: add --max-input-size= option

[Jeff King]

On Mon, Aug 15, 2016 at 09:57:28PM +0200, Christian Couder wrote:

> When receiving a pack-file, it can be useful to abort the
> `git unpack-objects`, if the pack-file is too big.
> 
> Signed-off-by: Christian Couder 

Same remarks here as on the last patch, including strtoumax. :)

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

Re: [RFC/PATCH 1/3] index-pack: add --max-input-size= option

[Jeff King]

On Mon, Aug 15, 2016 at 09:57:27PM +0200, Christian Couder wrote:

> From: Jeff King 
> 
> When receiving a pack-file, it can be useful to abort the
> `git index-pack`, if the pack-file is too big.

Not much rationale here. I guess because it is all in the 3rd patch,
which ties it into receive-pack. I'm not sure it's worth repeating. I
guess it could all be squished back into one patch. I'm OK either way.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 1d2ea58..1fd60bd 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -87,6 +87,7 @@ static struct progress *progress;
>  static unsigned char input_buffer[4096];
>  static unsigned int input_offset, input_len;
>  static off_t consumed_bytes;
> +static off_t max_input_size;
>  static unsigned deepest_delta;
>  static git_SHA_CTX input_ctx;
>  static uint32_t input_crc32;
> @@ -297,6 +298,8 @@ static void use(int bytes)
>   if (signed_add_overflows(consumed_bytes, bytes))
>   die(_("pack too large for current definition of off_t"));
>   consumed_bytes += bytes;
> + if (max_input_size && consumed_bytes > max_input_size)
> + die(_("pack exceeds maximum allowed size"));

Looks good. I see you marked it for translation, which makes sense.

On the original, I waffled on whether to share the size with the user in
the message. I didn't want to encourage people with "oh, if it's under
2G it must be OK, then!". Because really 2G was meant to be a "you
really shouldn't get this high, and we will unceremoniously dump your
push if you do".

>  static const char *open_pack_file(const char *pack_name)
> @@ -1714,6 +1717,8 @@ int cmd_index_pack(int argc, const char **argv, const 
> char *prefix)
>   opts.off32_limit = strtoul(c+1, , 0);
>   if (*c || opts.off32_limit & 0x8000)
>   die(_("bad %s"), arg);
> + } else if (skip_prefix(arg, "--max-input-size=", )) 
> {
> + max_input_size = strtoul(arg, NULL, 10);

max_input_size is an off_t, but your parse only up to ULONG_MAX here.
For my purposes in the original patch, this was OK, as we set it at 2GB,
which works everywhere (and also, GitHub systems all have 64-bit "long"
these days). But somebody on a 32-bit system could not set this to 4GB,
even though I think index-pack could otherwise handle it. We seem to use
strtoumax() elsewhere, so that's probably a good match (technically it
can overflow an off_t, but in practice this value comes from the admin
and they will set something sane).

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

Re: storing cover letter of a patch series?

[Jacob Keller]

On Mon, Aug 15, 2016 at 5:37 AM, Philip Oakley  wrote:
> [sorry if this is not the right place to 'drop in'..]
> I appreciate there has been a lot of discussion, but it mainly appears to be
> about an upstream / integration viewpoint.
>
> I'd hate it if there was a one size fits all solution that was only focused
> on one important use case, rather than having at least a simple fallback for
> simple folk.
>
> Personally I liked the idea that I could start my patch series branch with a
> simple 'empty' commit with a commit message that read "cover!  the series>" and continue with the cover letter. It's essentially the same
> as the fixup! and squash! idea (more the latter - it's squash! without a
> predecessor). For moderate size series a simple 'git rebase master..' is
> sufficient to see the whole series and decide which need editing, rewording,
> swapping, checking the fixups, etc.
>
> Format-patch would then be taught to spot that the first commit in the
> series is "cover! " and create the usual 0/N cover letter. Git Gui
> may need to be taught to recognise cover! (haven't checked if it recognises
> an empty commit squash!). Possibly 'git commit' may want a --cover option to
> massage the commit message and add --allow-empty, but that's finesse.
>
> I've no problem with more extensive methods for those preparing very big
> patch series, or with those needing to merge together a lot of series and
> want to keep the cover letters, but ensuring that a simple flow is possible
> should still be there.
> --
> Philip
>

Some people have suggested this simple idea, and I like it, but they
did mention that modifying the cover letter now requires a rebase over
a potentially large series of patches, which can get annoying.

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

Re: [ANNOUNCE] git-series: track changes to a patch series over time

[Josh Triplett]

On Mon, Aug 15, 2016 at 12:17:11PM -0600, Simon Glass wrote:
> On 1 August 2016 at 12:37, Josh Triplett  wrote:
> > On Mon, Aug 01, 2016 at 09:14:54AM -0600, Stephen Warren wrote:
> >> On 07/29/2016 12:40 AM, Josh Triplett wrote:
> >> > I'd like to announce a project I've been working on for a while:
> >> >
> >> > git-series provides a tool for managing patch series with git, tracking
> >> > the "history of history". git series tracks changes to the patch series
> >> > over time, including rebases and other non-fast-forwarding changes. git
> >> > series also tracks a cover letter for the patch series, formats the
> >> > series for email, and prepares pull requests.
> >>
> >> Just as an FYI, I wouldn't be surprised if there's some overlap, or
> >> potential for merging of tools, between this tool and the "patman" tool
> >> that's part of the U-Boot source tree:
> >>
> >> http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;h=e36857dedea1d0dbafa41732aaf9bf0988d63f38;hb=HEAD
> >
> > Interesting tool; thanks for the link.
> >
> > As far as I can tell from that documentation, patman doesn't track old
> > versions of a patch series; you rebase to modify patches or change
> > patman tags (embedded in commit messages), and nothing preserves the
> > previous version.  And it tracks the cover letter and similar in one of
> > the commit messages in the series, so previous versions of that don't
> > get saved either.  If you wanted to track the history of your changes,
> > you'd have to use branch names or similar.
> 
> That's right. Normally you would keep the old branch around, or tag
> it. Of course old branches are often based on older versions the
> upstream repo, so they are not that useful for diiff, etc. But the
> normal procedure when updating a series to a new version is:
> 
> git checkout -b wibble-v2 wibble
> git rebase upstream/master

That's the workflow I used before git-series, as well.  Having to create
versioned branch names motivated creating git-series; the branch names
in the git-series documentation ("feature-v8-rebased-4.6-alice-fix") are
*reduced* versions of actual branch names used for internal projects.

> > In addition, tracking metadata in commit messages only works with a
> > patches-by-mail workflow where the messages get processed when
> > generating patches; that doesn't work for please-pull workflows.
> 
> Can you explain what a please-pull workflow looks like, and what tags
> are expected?

You push the branch somewhere, as a branch or tag, and then use git
request-pull or otherwise tell someone "please pull from here".  They
pull the *exact* commit hashes you pushed, including whatever you based
them on.  That means they get the exact commit messages you pushed.  So,
if you have any inline metadata in the commit message, that would end up
in the project history.  The Linux kernel and other projects object to
getting those kinds of bits in commit messages; I've seen many patches
rejected because they included a Gerrit Change-Id.

Tracking the history and cover letter in a separate string of "series
commits" allows the underlying patch series to contain the exact commits
you want upstream to pull, without any postprocessing required.

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

[RFC/PATCH 2/3] unpack-objects: add --max-input-size= option

[Christian Couder]

When receiving a pack-file, it can be useful to abort the
`git unpack-objects`, if the pack-file is too big.

Signed-off-by: Christian Couder 
---
 builtin/unpack-objects.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 172470b..59b1f39 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -19,6 +19,7 @@ static const char unpack_usage[] = "git unpack-objects [-n] 
[-q] [-r] [--strict]
 static unsigned char buffer[4096];
 static unsigned int offset, len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static git_SHA_CTX ctx;
 static struct fsck_options fsck_options = FSCK_OPTIONS_STRICT;
 
@@ -87,6 +88,8 @@ static void use(int bytes)
if (signed_add_overflows(consumed_bytes, bytes))
die("pack too large for current definition of off_t");
consumed_bytes += bytes;
+   if (max_input_size && consumed_bytes > max_input_size)
+   die(_("pack exceeds maximum allowed size"));
 }
 
 static void *get_data(unsigned long size)
@@ -550,6 +553,10 @@ int cmd_unpack_objects(int argc, const char **argv, const 
char *prefix)
len = sizeof(*hdr);
continue;
}
+   if (skip_prefix(arg, "--max-input-size=", )) {
+   max_input_size = strtoul(arg, NULL, 10);
+   continue;
+   }
usage(unpack_usage);
}
 
-- 
2.10.0.rc0.4.g229e32c.dirty

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

[RFC/PATCH 3/3] receive-pack: allow a maximum input size to be specified

[Christian Couder]

From: Jeff King 

Receive-pack feeds its input to either index-pack or
unpack-objects, which will happily accept as many bytes as
a sender is willing to provide. Let's allow an arbitrary
cutoff point where we will stop writing bytes to disk.

What has already been written to disk can be cleaned
outside of receive-pack.

Signed-off-by: Jeff King 
Signed-off-by: Christian Couder 
---
 builtin/receive-pack.c | 12 
 t/t5546-push-limits.sh | 47 +++
 2 files changed, 59 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 92e1213..7627f7f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;
 static int advertise_atomic_push = 1;
 static int advertise_push_options;
 static int unpack_limit = 100;
+static off_t max_input_size;
 static int report_status;
 static int use_sideband;
 static int use_atomic;
@@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const char 
*value, void *cb)
return 0;
}
 
+   if (strcmp(var, "receive.maxsize") == 0) {
+   max_input_size = git_config_ulong(var, value);
+   return 0;
+   }
+
return git_default_config(var, value, cb);
 }
 
@@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
if (fsck_objects)
argv_array_pushf(, "--strict%s",
fsck_msg_types.buf);
+   if (max_input_size)
+   argv_array_pushf(, "--max-input-size=%lu",
+   max_input_size);
child.no_stdout = 1;
child.err = err_fd;
child.git_cmd = 1;
@@ -1678,6 +1687,9 @@ static const char *unpack(int err_fd, struct shallow_info 
*si)
fsck_msg_types.buf);
if (!reject_thin)
argv_array_push(, "--fix-thin");
+   if (max_input_size)
+   argv_array_pushf(, "--max-input-size=%lu",
+   max_input_size);
child.out = -1;
child.err = err_fd;
child.git_cmd = 1;
diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
new file mode 100755
index 000..d3a4d1a
--- /dev/null
+++ b/t/t5546-push-limits.sh
@@ -0,0 +1,47 @@
+#!/bin/sh
+
+test_description='check input limits for pushing'
+. ./test-lib.sh
+
+test_expect_success 'create known-size commit' '
+   test-genrandom foo 1024 >file &&
+   git add file &&
+   test_commit one-k
+'
+
+test_expect_success 'create remote repository' '
+   git init --bare dest &&
+   git --git-dir=dest config receive.unpacklimit 1
+'
+
+test_expect_success 'receive.maxsize rejects push' '
+   git --git-dir=dest config receive.maxsize 512 &&
+   test_must_fail git push dest HEAD
+'
+
+test_expect_success 'bumping limit allows push' '
+   git --git-dir=dest config receive.maxsize 4k &&
+   git push dest HEAD
+'
+
+test_expect_success 'create another known-size commit' '
+   test-genrandom bar 2048 >file2 &&
+   git add file2 &&
+   test_commit two-k
+'
+
+test_expect_success 'change unpacklimit' '
+   git --git-dir=dest config receive.unpacklimit 10
+'
+
+test_expect_success 'receive.maxsize rejects push' '
+   git --git-dir=dest config receive.maxsize 512 &&
+   test_must_fail git push dest HEAD
+'
+
+test_expect_success 'bumping limit allows push' '
+   git --git-dir=dest config receive.maxsize 4k &&
+   git push dest HEAD
+'
+
+test_done
-- 
2.10.0.rc0.4.g229e32c.dirty

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

[RFC/PATCH 1/3] index-pack: add --max-input-size= option

[Christian Couder]

From: Jeff King 

When receiving a pack-file, it can be useful to abort the
`git index-pack`, if the pack-file is too big.

Signed-off-by: Jeff King 
Signed-off-by: Christian Couder 
---
 builtin/index-pack.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1d2ea58..1fd60bd 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -87,6 +87,7 @@ static struct progress *progress;
 static unsigned char input_buffer[4096];
 static unsigned int input_offset, input_len;
 static off_t consumed_bytes;
+static off_t max_input_size;
 static unsigned deepest_delta;
 static git_SHA_CTX input_ctx;
 static uint32_t input_crc32;
@@ -297,6 +298,8 @@ static void use(int bytes)
if (signed_add_overflows(consumed_bytes, bytes))
die(_("pack too large for current definition of off_t"));
consumed_bytes += bytes;
+   if (max_input_size && consumed_bytes > max_input_size)
+   die(_("pack exceeds maximum allowed size"));
 }
 
 static const char *open_pack_file(const char *pack_name)
@@ -1714,6 +1717,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
opts.off32_limit = strtoul(c+1, , 0);
if (*c || opts.off32_limit & 0x8000)
die(_("bad %s"), arg);
+   } else if (skip_prefix(arg, "--max-input-size=", )) 
{
+   max_input_size = strtoul(arg, NULL, 10);
} else
usage(index_pack_usage);
continue;
-- 
2.10.0.rc0.4.g229e32c.dirty

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

[RFC/PATCH 0/3] limit the size of the packs we receive

[Christian Couder]

In https://public-inbox.org/git/20150612182045.GA23698%40peff.net/,
Peff sent a patch that is used by GitHub to abort `git receive-pack`
when the size of the pack we receive is bigger than a configured
limit.

GitLab is interested in using the same approach and in standardizing
the error messages the user could get back.

So I rebased Peff's patch to the current master, refreshed it a bit,
split it, and added the missing --max-input-size= option to
`git unpack-objects` - to make it work for all `transfer.unpacklimit`
values - in a new patch.

There is no documentation yet for the `--max-input-size=`
options added to `git index-pack` and `git unpack-objects`, nor for
the new `receive.maxsize` config option.

I kept Peff as the author of the patches that are made mostly from his
patch, but I added my Signed-off-by to them.

Christian Couder (1):
  unpack-objects: add --max-input-size= option

Jeff King (2):
  index-pack: add --max-input-size= option
  receive-pack: allow a maximum input size to be specified

 builtin/index-pack.c |  5 +
 builtin/receive-pack.c   | 12 
 builtin/unpack-objects.c |  7 +++
 t/t5546-push-limits.sh   | 47 +++
 4 files changed, 71 insertions(+)
 create mode 100755 t/t5546-push-limits.sh

-- 
2.10.0.rc0.4.g229e32c.dirty

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

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 11:54:53AM -0700, Stefan Beller wrote:

> > SoI guess. But has anybody in the history of git ever explicitly
> > configured advice.* to true?
> 
> An admin/teacher of a university that wants to teach Git to students
> in a class? Better be safe than sorry and explicitly ask for advice because...
> we cannot trust the default?
> 
> >
> > It has never produced any change of behavior, and the whole point of
> > "advice.*" was that git would advise by default, and you would use
> > advice.* to shut it up once you were sufficiently educated.
> 
> And now I am arguing that "by default" we should not give advice 100%
> of the time, but only when we think it is appropriate. You may disagree
> (as a teacher see above), so you can slightly change the setting to give
> out advice more often again?

I don't think it's quite the same thing. It is fine not to bother
advising because the advice is not really applicable, and that is what
is happening here. We do not need to lecture the user on something they
explicitly asked for.

But that is different than "by default, in situations where we have
useful advice to give, give it".

I guess you are indicating that somebody may disagree on "applicable"
here. Which I suppose is possible, but it seems like a bit of a made-up
hypothetical.


I had also thought at first you were arguing from the position of "let's
handle advice.detachedHEAD=true in case somebody has set it". That seems
even more silly, because almost certainly nobody _has_ set it. But if
your position is "let's make it do something useful in case somebody
wants to set it", then...I still think it's silly, but at least there is
room for debate. :)

> > I don't think doing it this way is _wrong_. It just feels sort of
> > pointlessly over-engineered. It's also a little weird that all of the:
> >
> >   if (advice_foo)
> >
> > will trigger because "advice_foo" is set to -1. I think it does the
> > right thing, but it feels like a bug (the value is now a tri-state, and
> > we silently collapse two states into one).
> 
> I think this is what I did in some of the submodule code as well, which
> is why I assumed it's ok (or rather the projects groupthink on how to do
> "default on but still different than explicit on")
> 
> If you think this is wrong, what is the idiomatic way to solve this problem?

I don't think it's wrong (didn't I say so :) ).

It's just that idiomatic use of a tri-state like this is generally
something like:

  1. Set the option to -1 for "not specified"

  2. Fill in the values as 0/1 if the user asks for it.

  3. Canonicalize any remaining unspecified value to 0/1, depending on
 the default. Usually this happens after we know all setup is done,
 but sometimes is done lazily by an accessor function.

  3. Check the canonical value with "if (option)", or "if (option())" if
 using a lazy accessor.

And we have fixed many bugs in the past where some non-canonical value
slipped past step 3, and did the wrong thing in step 4.

Here it's OK because "if (option)" means that "unspecified" collapses to
"true", and that is the default for each of these options. It's just
that it's hard to distinguish from the buggy case.

I suppose the more idiomatic way would be:

  static int advice_wanted(int x)
  {
if (advice_values[x] < 0)
advice_values[x] = 1; /* all advice defaults to on */
return advice_values[x];
  }

  ...
  if (advice_wanted(ADVICE_DETACHED_HEAD))
...

but perhaps it is not worth that amount of boilerplate (but maybe at
least a comment explaining the situation). You'd also need to check with
your patch that each user of the advice variables is checking just for a
non-zero value, and not explicitly for "1" (which is almost certainly
the case, but it needs to be considered).

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

Re: [PATCH] submodule: mark a file-local symbol as static

[Stefan Beller]

On Sat, Aug 13, 2016 at 7:31 AM, Ramsay Jones
 wrote:
>
> Signed-off-by: Ramsay Jones 
> ---
>
> Hi Stefan,
>
> If you need to re-roll your 'sb/submodule-clone-rr' branch, could
> you please squash this into the relevant patch (commit 7bcd1d17,
> "clone: recursive and reference option triggers submodule alternates",
> 11-08-2016).
>
> Thanks!

Thanks,

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

Re: [PATCHv4 5/8] clone: factor out checking for an alternate path

[Stefan Beller]

On Fri, Aug 12, 2016 at 3:25 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> + struct strbuf sb = STRBUF_INIT;
>> + char *ref_git = compute_alternate_path(item->string, );
>
> Who owns the memory for ref_git?

The caller of compute_alternate_path(..), which makes
add_one_reference faulty as of this patch.

>
>> - if (!access(mkpath("%s/shallow", ref_git), F_OK))
>> - die(_("reference repository '%s' is shallow"), item->string);
>> + if (!ref_git)
>> + die("%s", sb.buf);
>
> Presumably the second argument to compute_alternate_path() is a
> strbuf to receive the error message?  It is unfortunate that the
> variable used for this purpose is a bland "sb", but perhaps that
> cannot be helped as you would reuse that strbuf for a different
> purpose (i.e. not to store the error message, but to formulate a
> pathname).

Ok. I had an intermediate version with 2 strbufs but for some reason I
decided one is better. We'll have 2 again. (err and sb; sb will have a
smaller scope only in the else part.)

>
>> - if (!access(mkpath("%s/info/grafts", ref_git), F_OK))
>> - die(_("reference repository '%s' is grafted"), item->string);
>> + strbuf_addf(, "%s/objects", ref_git);
>> + add_to_alternates_file(sb.buf);
>>
>> - strbuf_addf(, "%s/objects", ref_git);
>> - add_to_alternates_file(alternate.buf);
>> - strbuf_release();
>> - free(ref_git);
>> + strbuf_release();
>
> I am wondering about the loss of free() here in the first comment.

fixed in a reroll.

>
>> +/*
>> + * Compute the exact path an alternate is at and returns it. In case of
>> + * error NULL is returned and the human readable error is added to `err`
>> + * `path` may be relative and should point to $GITDIR.
>> + * `err` must not be null.
>> + */
>> +char *compute_alternate_path(const char *path, struct strbuf *err)
>> +{
>> + char *ref_git = NULL;
>> + const char *repo, *ref_git_s;
>> + struct strbuf err_buf = STRBUF_INIT;
>
> Why do you need "err_buf", instead of directly writing the error to
> "err", especially if "err" is not optional?
>
>> + ...
>> +out:
>> + if (err_buf.len) {

If we were directly writing to err, we would have checked
err.len here. Then you open up a subtle way of saying "dry run"
by giving a non empty error buffer.

I contemplated doing that actually instead of splitting up into 2 functions,
but I considered that bad taste as it would require documentation.

>> + strbuf_addbuf(err, _buf);
>> + free(ref_git);
>> + ref_git = NULL;
>> + }
>> +
>> + strbuf_release(_buf);
>> + return ref_git;
>> +}
>
> So ref_git is a piece of memory on heap, and the caller is
> responsible for not leaking it.

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

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Mon, Aug 15, 2016 at 11:28:20AM -0700, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:
> >
> >> On Aug 15 2016, Jeff King  wrote:
> >> 
> >> > And implicit in your test is the other bug, which is that deleting the
> >> > last key in a section leaves the empty header. I think it's related to
> >> > the same issue.
> >> 
> >> Indiscriminately removing empty section headers may break comments that
> >> have been put there on purpose.
> >
> > I know, but we do not even do so discriminately.
> 
> I notice that we have thought about all the issues when we last
> discussed it in 2013.  Refining a message from the earlier thread,
> as it illustrates tricky cases in which we have to be careful.

Thanks for digging up the threads that I was too lazy to find.

I agree with most everything here, though I would be happy if somebody
even wrote a patch to handle the "easy" cases.

> So a comment outside [section "name"] is tricky; it needs some
> mechanism (or convention) to tell us if it is about the particular
> section, or it is about the location in the configuration file.

Keep in mind that even "outside" is hard, because sections do not
explicitly close.

So in:

  [core]
  foo = bar

  # here are my remotes

  [remote "github"]
  url = ...

How do we know that the comment is "outside" and not part of [core]?

We can perhaps guess so because there are no keys after it in the
section, though there are some special cases, like:

  [core]
  foo = bar
  # This isn't necessary anymore because...
  # xyzzy = false

or even:

  [core]
  foo = bar # needed because of xyzzy

You can probably make reasonable cases based on heuristics around
newlines, but that is even further into "convention" territory.

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

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

On Mon, Aug 15, 2016 at 11:47 AM, Jeff King  wrote:
> On Mon, Aug 15, 2016 at 11:40:21AM -0700, Stefan Beller wrote:
>
>> When a user asked for a detached HEAD specifically with `--detach`,
>> we do not need to give advice on what a detached HEAD state entails as
>> we can assume they know what they're getting into as they asked for it.
>>
>> Signed-off-by: Stefan Beller 
>> ---
>>
>>  Junio writes:
>>  > It might be controversial how the second from the last case should
>>  > behave, though.
>>
>>  I agree. I think if the advice is configured explicitly we can still give 
>> it.
>>  That makes the code a bit more complicated though.
>
> SoI guess. But has anybody in the history of git ever explicitly
> configured advice.* to true?

An admin/teacher of a university that wants to teach Git to students
in a class? Better be safe than sorry and explicitly ask for advice because...
we cannot trust the default?

>
> It has never produced any change of behavior, and the whole point of
> "advice.*" was that git would advise by default, and you would use
> advice.* to shut it up once you were sufficiently educated.

And now I am arguing that "by default" we should not give advice 100%
of the time, but only when we think it is appropriate. You may disagree
(as a teacher see above), so you can slightly change the setting to give
out advice more often again?

>
> I don't think doing it this way is _wrong_. It just feels sort of
> pointlessly over-engineered. It's also a little weird that all of the:
>
>   if (advice_foo)
>
> will trigger because "advice_foo" is set to -1. I think it does the
> right thing, but it feels like a bug (the value is now a tri-state, and
> we silently collapse two states into one).

I think this is what I did in some of the submodule code as well, which
is why I assumed it's ok (or rather the projects groupthink on how to do
"default on but still different than explicit on")

If you think this is wrong, what is the idiomatic way to solve this problem?

Thanks,
Stefan


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

Re: Minor bug: git config ignores empty sections

[Andreas Schwab]

On Aug 15 2016, Junio C Hamano  wrote:

> If we were to remove the section header at this point, we should
> be removing the comment two out of three cases.
>
>  - if it is about section.key, it should go when section.key goes.
>  - if it is about section, it should go when section goes.
>  - if it is a more generic comment about this configuration file,
>it should stay.

The comment may not be related to the surrounding keys, but just a
commented-out config key that you want to keep around for reference.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 11:40:21AM -0700, Stefan Beller wrote:

> When a user asked for a detached HEAD specifically with `--detach`,
> we do not need to give advice on what a detached HEAD state entails as
> we can assume they know what they're getting into as they asked for it.
> 
> Signed-off-by: Stefan Beller 
> ---
> 
>  Junio writes:
>  > It might be controversial how the second from the last case should
>  > behave, though.
>  
>  I agree. I think if the advice is configured explicitly we can still give it.
>  That makes the code a bit more complicated though.

SoI guess. But has anybody in the history of git ever explicitly
configured advice.* to true?

It has never produced any change of behavior, and the whole point of
"advice.*" was that git would advise by default, and you would use
advice.* to shut it up once you were sufficiently educated.

I don't think doing it this way is _wrong_. It just feels sort of
pointlessly over-engineered. It's also a little weird that all of the:

  if (advice_foo)

will trigger because "advice_foo" is set to -1. I think it does the
right thing, but it feels like a bug (the value is now a tri-state, and
we silently collapse two states into one).

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

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Jeff King]

On Mon, Aug 15, 2016 at 09:57:52AM -0700, Junio C Hamano wrote:

> I wonder if we already have a good mechanism to allow a project and
> its participants (say, "me") to declare "in this project, pathnames
> must conform to this rule" and help them avoid creating a tree that
> violates the rule customized to their project.
> 
> I guess "write_index_as_tree()" would be one of the central places
> to hook into and that covers an individual contributor or a patch
> applier who ends up adding offending paths to the project, as well
> as a merge made in response to a pull request (unless it is a
> fast-forward) [*1*].  The pre-receive hook can also be used to
> inspect and reject an attempt to push an offending tree into the
> history.
> 
> Such a mechanism would allow a project that wants participation by
> folks with case insensitive filesystems to ensure that they do not
> create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
> same time, for example, but the mechanism needs to allow visibility
> into more than just a single path when the custom check is made
> (e.g. a hook run in "write_index_as_tree()" can see all entries in
> the index to make the decision; if we were to also hook into
> "add_to_index()", the hook must be able to see other entries in the
> index to which the new entry is being added).

I am not convinced this mechanism needs to be built into git. Because it
happens to be about filenames, git at least has a hope of making sense
of the various project rules.

But conceptually, I don't think this is really any different than "don't
check in code which does not build on platform X", or "do not check in
code that does not meet our style guide". We already have general hooks
where such checks can be made[1], and this could be checked in the same
place.

I actually think the whitespace-checking done by diff and apply is in a
similar boat, though it is useful in practice. OTOH, I think primarily
it is used as a tool to feed information to policy hooks, rather than as
an enforcement mechanism itself (maybe --whitespace=fix on apply is an
exception there, though).

-Peff

[1] Obviously we have pre-commit for local enforcement and pre-receive
for central enforcement. But people with workflows around CI-style
tests would want to be able to fetch, check "does this meet our
policy", and return a yes/no answer on whether the result is OK to
merge.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] advice: preset advice preferences to -1

[Stefan Beller]

In the next patch we want to make a distinction if a the advice was
requested explicitly or is set implicit. To do that we need to have
different values for the default and a value that is set explicitly.

Signed-off-by: Stefan Beller 
---
 advice.c | 30 +++---
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/advice.c b/advice.c
index b84ae49..56b70b6 100644
--- a/advice.c
+++ b/advice.c
@@ -1,20 +1,20 @@
 #include "cache.h"
 
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_commit_before_merge = 1;
-int advice_resolve_conflict = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_rm_hints = 1;
+int advice_push_update_rejected = -1;
+int advice_push_non_ff_current = -1;
+int advice_push_non_ff_matching = -1;
+int advice_push_already_exists = -1;
+int advice_push_fetch_first = -1;
+int advice_push_needs_force = -1;
+int advice_status_hints = -1;
+int advice_status_u_option = -1;
+int advice_commit_before_merge = -1;
+int advice_resolve_conflict = -1;
+int advice_implicit_identity = -1;
+int advice_detached_head = -1;
+int advice_set_upstream_failure = -1;
+int advice_object_name_warning = -1;
+int advice_rm_hints = -1;
 
 static struct {
const char *name;
-- 
2.9.2.730.g525ad04.dirty

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

[PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

When a user asked for a detached HEAD specifically with `--detach`,
we do not need to give advice on what a detached HEAD state entails as
we can assume they know what they're getting into as they asked for it.

Signed-off-by: Stefan Beller 
---

 Junio writes:
 > It might be controversial how the second from the last case should
 > behave, though.
 
 I agree. I think if the advice is configured explicitly we can still give it.
 That makes the code a bit more complicated though.
 
 Also note I added stderr to stdout redirections as suggested by Peff.
 
 Thanks,
 Stefan
 
 builtin/checkout.c |  4 +++-
 t/t2020-checkout-detach.sh | 28 
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4866111..6196b40 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -658,7 +658,9 @@ static void update_refs_for_switch(const struct 
checkout_opts *opts,
update_ref(msg.buf, "HEAD", new->commit->object.oid.hash, NULL,
   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
if (!opts->quiet) {
-   if (old->path && advice_detached_head)
+   if (old->path &&
+   (advice_detached_head == 1 ||
+(advice_detached_head == -1 && 
!opts->force_detach)))
detach_advice(new->name);
describe_detached_head(_("HEAD is now at"), 
new->commit);
}
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 5d68729..fe311a1 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -163,4 +163,32 @@ test_expect_success 'tracking count is accurate after 
orphan check' '
test_i18ncmp expect stdout
 '
 
+test_expect_success 'no advice given for explicit detached head state' '
+   # baseline
+   test_config advice.detachedHead true &&
+   git checkout child && git checkout HEAD^0 >expect.advice 2>&1 &&
+   test_config advice.detachedHead false &&
+   git checkout child && git checkout HEAD^0 >expect.no-advice 2>&1 &&
+   test_unconfig advice.detachedHead &&
+   # without configuration, the advice.* variables default to true
+   git checkout child && git checkout HEAD^0 >actual 2>&1 &&
+   test_cmp expect.advice actual &&
+
+   # with explicit --detach
+   # no configuration
+   test_unconfig advice.detachedHead &&
+   git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+   test_cmp expect.no-advice actual &&
+
+   # explicitly ask advice
+   test_config advice.detachedHead true &&
+   git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+   test_cmp expect.advice actual &&
+
+   # explicitly decline advice
+   test_config advice.detachedHead false &&
+   git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+   test_cmp expect.no-advice actual
+'
+
 test_done
-- 
2.9.2.730.g525ad04.dirty

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

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Jeff King  writes:

> On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:
>
>> On Aug 15 2016, Jeff King  wrote:
>> 
>> > And implicit in your test is the other bug, which is that deleting the
>> > last key in a section leaves the empty header. I think it's related to
>> > the same issue.
>> 
>> Indiscriminately removing empty section headers may break comments that
>> have been put there on purpose.
>
> I know, but we do not even do so discriminately.

I notice that we have thought about all the issues when we last
discussed it in 2013.  Refining a message from the earlier thread,
as it illustrates tricky cases in which we have to be careful.

If we were to remove the section header at this point, we should
be removing the comment two out of three cases.

 - if it is about section.key, it should go when section.key goes.
 - if it is about section, it should go when section goes.
 - if it is a more generic comment about this configuration file,
   it should stay.

A configuration file may have something like this:

# Now, let's list all the remotes I interact with.

# This is where I push all the separate topics.
[remote "github"]
# fetch like everybody else without auth
url = git://github.com/user/git
# push with auth
pushURL = github.com:user/git
# publish my working area as-is
mirror

# Traditional "canonical" one
[remote "korg"]
url = k.org:/pub/scm/user/git

If I were to retire the "github" account, removing the entire
[remote "github"] section while keeping the comments before that
section would be confusing, which would end up with:

# Now, let's list all the remotes I interact with.

# This is where I push all the separate topics.

# Traditional "canonical" one
[remote "korg"]
url = k.org:/pub/scm/user/git

because I do not push all the separate topics to korg.

Removing the section while removing the comments that pertain to the
section is impossible; "Now, let's list all the remotes" should
stay, "This is where I push" should go.

So a comment outside [section "name"] is tricky; it needs some
mechanism (or convention) to tell us if it is about the particular
section, or it is about the location in the configuration file.

Removing only the keys and keeping the skelton around would give us:

# Now, let's list all the remotes I interact with.
# This is where I push all the separate topics.
[remote "github"]
# fetch like everybody else without auth
# push with auth
# publish my working area as-is

# Traditional "canonical" one
[remote "korg"]
url = k.org:/pub/scm/user/git

which is still confusing, but at least the confusion is not spread
to adjacent sections.  What we could do easily without understanding
natural languages is to remove comments inside [section "name"], but
it still depends on a convention that such a per-key comment comes
before the key, not after, i.e.

# This is where I push all the separate topics.
[remote "github"]
# fetch like everybody else without auth
...
# publish my working area as-is
mirror
# note that mirror may push out outside tags/ and heads/

# Traditional "canonical" one
[remote "korg"]

We could also take hints from the indentation level, but now we are
deep into the "convention" territory once we start doing that.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Draft of Git Rev News edition 18

[Jakub Narębski]

W dniu 15.08.2016 o 20:08, Jakub Narębski pisze:
[...]
> Additional references / information:
> 
> * "Alternatives to mid.gmane.org?" thread, starting with
>   Message-Id: <481d1ee2-6a66-418f-ab28-95947bbf3...@gmail.com>
> 
>   Mentions a few alternatives besides public-inbox:
> 
>  https://marc.info/?i=%s
>  https://mid.mail-archive.com/%s
>  https://lists.debian.org/msgid-search/%s
> 
>   for finding message based on Message-Id
> 
>   NOTE: there are a few mailing list archive sites not mentioned
>   in this thread, like Nabble, some of them listed on (not updated)
>   https://git.wiki.kernel.org/index.php/GitCommunity
> 
> * "Ingebrigtsen: The End of Gmane?" short note / article on LWN.net,
>   https://lwn.net/Articles/695695/ [Posted July 28, 2016 by jake],
>   part of "Announcements" (http://lwn.net/Articles/695980/) section
>   of LWN.net Weekly Edition for August 4, 2016 
>   (http://lwn.net/Articles/695974/)

* "A note from the maintainer" from 12 Aug 2016
  Message-Id: 
  
  includes updated links to mailing list archives, post-Gmane,
  and some discussion about public-archive.org

-- 
Jakub Narębski

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

Re: [ANNOUNCE] git-series: track changes to a patch series over time

[Simon Glass]

Hi Josh,

On 1 August 2016 at 12:37, Josh Triplett  wrote:
> On Mon, Aug 01, 2016 at 09:14:54AM -0600, Stephen Warren wrote:
>> On 07/29/2016 12:40 AM, Josh Triplett wrote:
>> > I'd like to announce a project I've been working on for a while:
>> >
>> > git-series provides a tool for managing patch series with git, tracking
>> > the "history of history". git series tracks changes to the patch series
>> > over time, including rebases and other non-fast-forwarding changes. git
>> > series also tracks a cover letter for the patch series, formats the
>> > series for email, and prepares pull requests.
>>
>> Just as an FYI, I wouldn't be surprised if there's some overlap, or
>> potential for merging of tools, between this tool and the "patman" tool
>> that's part of the U-Boot source tree:
>>
>> http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README;h=e36857dedea1d0dbafa41732aaf9bf0988d63f38;hb=HEAD
>
> Interesting tool; thanks for the link.
>
> As far as I can tell from that documentation, patman doesn't track old
> versions of a patch series; you rebase to modify patches or change
> patman tags (embedded in commit messages), and nothing preserves the
> previous version.  And it tracks the cover letter and similar in one of
> the commit messages in the series, so previous versions of that don't
> get saved either.  If you wanted to track the history of your changes,
> you'd have to use branch names or similar.

That's right. Normally you would keep the old branch around, or tag
it. Of course old branches are often based on older versions the
upstream repo, so they are not that useful for diiff, etc. But the
normal procedure when updating a series to a new version is:

git checkout -b wibble-v2 wibble
git rebase upstream/master
git commit --amend
# Edit commit to add 'Series-version: 2', update cover letter etc.

Of course any change log is preserved when you move to v3, since you
just add more 'Series-changes:' tags. The old version of the cover
letter, and the old version of the commits can be preserved with 'git
tag'.

>
> In addition, tracking metadata in commit messages only works with a
> patches-by-mail workflow where the messages get processed when
> generating patches; that doesn't work for please-pull workflows.

Can you explain what a please-pull workflow looks like, and what tags
are expected?

>
> patman does have quite a few interesting ideas, though.  git-series
> needs some way of handling To/Cc addresses for patches and the cover
> letter (beyond just scripts/get_maintainer.pl), and more automatic
> handling of series versioning (v2, v3, ...) and associated series
> changelogs.  Suggestions welcome.

Patman builds the cover letter change lists from the commits. The main
point of patman is to automate the error-prone process of submitting a
perfectly formed patch series.

In particular, patman requires no change to the normal workflow that
people use with git.

>
> - Josh Triplett

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

Re: Minor bug: git config ignores empty sections

[Jeff King]

On Mon, Aug 15, 2016 at 07:34:50PM +0200, Andreas Schwab wrote:

> On Aug 15 2016, Jeff King  wrote:
> 
> > And implicit in your test is the other bug, which is that deleting the
> > last key in a section leaves the empty header. I think it's related to
> > the same issue.
> 
> Indiscriminately removing empty section headers may break comments that
> have been put there on purpose.

I know, but we do not even do so discriminately.

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

Re: Draft of Git Rev News edition 18

[Jakub Narębski]

W dniu 14.08.2016 o 23:10, Philip Oakley pisze:
> From: "Christian Couder" 
>>
>> A draft of a new Git Rev News edition is available here:
>>
>>
>> https://github.com/git/git.github.io/blob/master/rev_news/drafts/edition-18.md
>>
>> Everyone is welcome to contribute in any section either by editing the
>> above page on GitHub and sending a pull request, or by commenting on
>> this GitHub issue:
>>
>>  https://github.com/git/git.github.io/issues/170
>>
>> You can also reply to this email.
> 
> I see you mention in passing (well in the small headings near the bottom)
> that gmane web interface has gone away. It may be worth noting a few of
> the alternatives, and in particular Eric's newly updated public-inbox
> https://public-inbox.org/git/.

It would be nice to turn it into mini-story rather than just putting
a link.

Additional references / information:

* "Alternatives to mid.gmane.org?" thread, starting with
  Message-Id: <481d1ee2-6a66-418f-ab28-95947bbf3...@gmail.com>

  Mentions a few alternatives besides public-inbox:

 https://marc.info/?i=%s
 https://mid.mail-archive.com/%s
 https://lists.debian.org/msgid-search/%s

  for finding message based on Message-Id

  NOTE: there are a few mailing list archive sites not mentioned
  in this thread, like Nabble, some of them listed on (not updated)
  https://git.wiki.kernel.org/index.php/GitCommunity

* "Ingebrigtsen: The End of Gmane?" short note / article on LWN.net,
  https://lwn.net/Articles/695695/ [Posted July 28, 2016 by jake],
  part of "Announcements" (http://lwn.net/Articles/695980/) section
  of LWN.net Weekly Edition for August 4, 2016 
  (http://lwn.net/Articles/695974/)

> I've found it very useful and probably easier to use.
> (now I've seen https://public-inbox.org/design_www.html)

It's a great pity that https://public-inbox.org/ is just
directory index, not a true home page.

-- 
Jakub Narębski

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

Re: [PATCH v2] help: make option --help open man pages only for Git commands

[Junio C Hamano]

"Philip Oakley"  writes:

> I'm still not sure this is enough. One of the problems back when I
> introduced the --guides option (65f9835 (builtin/help.c: add --guide
> option, 2013-04-02)) was that we had no easy way of determining what
> guides were available, especially given the *nix/Windows split where
> the help defaults are different (--man/--html).
>
> At the time[1] we (I) punted on trying to determine which guides were
> actually installed, and just created a short list of the important
> guides, which I believe you now check. However the less common guides
> are still there (gitcvs-migration?), and others may be added locally.

I think we should do both; "git help cvs-migration" should keep the
same codeflow and behaviour as we have today (so that it would still
work), while "git cvs-migration --help" should say "'cvs-migration'
is not a git command".  That would be a good clean-up anyway.

It obviously cannot be done if git.c::handle_builtin() does the same
"swap  --help to help " hack, but we could improve that
part (e.g. rewrite it to "help --swapped " to allow cmd_help()
to notice).  When the user said " --help", we don't do guides,
when we swapped the word order, we check with guides, too.


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

Re: BUG: indent-with-non-tab always on

[Marc Branchaud]

On 2016-08-15 12:55 PM, Marc Branchaud wrote:

On 2016-08-15 11:00 AM, Philip Oakley wrote:

From: "Marc Branchaud" 

On 2016-08-12 07:45 PM, Philip Oakley wrote:

These are the quick fixes to Marc's comments to patches 5,6,11,
and a consequential change to 12.

Only the changed patches are included.


Looks good to me -- no further comments!

However, I still don't understand why git says 11/12 has "indent with
spaces" problems.


I'm guessing it's that the text is monospaced rather than tabbed, and
it's flagging that.

I'd noticed that it was highlighted in the Git gui (which I use to
visualise both the diff, the message and the part after the three dashes
at the same time).


Actually, it looks like an indent-with-non-tab problem, which is
supposed to be off by default.

Git doesn't complain about the patch if I set
core.whitespace = tabwidth=11
presumably because the patch uses 10 spaces to indent the offending lines.

But explicitly disabling that check with
core.whitespace = -indent-with-non-tab
doesn't work.

Unfortunately, I don't have time today to track this down.


Gah, never mind -- didn't realize it was turned on in 
Documentation/.gitattributes.


M.

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

Re: Minor bug: git config ignores empty sections

[Junio C Hamano]

Eli Barzilay  writes:

> Looks like there's a problem with setting a config with an empty
> section, making it create a new section.  The result is:

Thanks; this unfortunately is a well-known issue, that once was even
(supposed to be) a part of GSoC project but hasn't been solved.

The latest mention of the same issue I think is

https://public-inbox.org/git/xmqqeg95aor6@gitster.mtv.corp.google.com/

which redirects to

If you are interested, start here:

http://thread.gmane.org/gmane.comp.version-control.git/219505/focus=219524

but gmane web interface is not working and an equivalent is here:

https://public-inbox.org/git/20130329195155.ga19...@sigill.intra.peff.net/

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

Re: Minor bug: git config ignores empty sections

[Andreas Schwab]

On Aug 15 2016, Jeff King  wrote:

> And implicit in your test is the other bug, which is that deleting the
> last key in a section leaves the empty header. I think it's related to
> the same issue.

Indiscriminately removing empty section headers may break comments that
have been put there on purpose.

Andreas.

-- 
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 00/12] Update git revisions

[Junio C Hamano]

Marc Branchaud  writes:

> On 2016-08-12 07:45 PM, Philip Oakley wrote:
>> These are the quick fixes to Marc's comments to patches 5,6,11,
>> and a consequential change to 12.
>>
>> Only the changed patches are included.
>
> Looks good to me -- no further comments!
>
> However, I still don't understand why git says 11/12 has "indent with
> spaces" problems.

Probably that is because Documentation/.gitattributes has

*.txt whitespace

to tell Git to notice all types of potential whitespace errors known
to it.  The checking of indent-with-tab is deliberate here, because
we rely on the fact that asciidoc treats HT as "fill with necessary
spaces to next multiple of 8" even when it renders a fixed-column
drawing.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Junio C Hamano  writes:

> Johannes Schindelin  writes:
>
>> Some file names that are okay on ext4 and on HFS+ are illegal in
>> Windows. In order to stay truly platform-independent, Git's source code
>> must not contain such illegal file names, even if things just happen to
>> work on Linux.
>
> Good thinking.
>
> Some tests may have to be skipped on platforms that cannot express
> certain paths, but even then they shouldn't ship a file with
> pathname that cannot even be checked out (they should instead create
> and use such a path, protected behind filesystem specific test
> prerequisite).

This is a tangent.

Adding a check target in t/Makefile is a good first step, but any
tree of a project that gets participation from those on different
platforms must allow any supported platforms to check it out.  The
issue is not limited to the t/ hierarchy, but the whole thing.  I
do not mean to say this patch needs to do more than checking t/ at
all, by the way.  After all, people send patches without running
test-lint so this only means that we as the project must be careful.

I wonder if we already have a good mechanism to allow a project and
its participants (say, "me") to declare "in this project, pathnames
must conform to this rule" and help them avoid creating a tree that
violates the rule customized to their project.

I guess "write_index_as_tree()" would be one of the central places
to hook into and that covers an individual contributor or a patch
applier who ends up adding offending paths to the project, as well
as a merge made in response to a pull request (unless it is a
fast-forward) [*1*].  The pre-receive hook can also be used to
inspect and reject an attempt to push an offending tree into the
history.

Such a mechanism would allow a project that wants participation by
folks with case insensitive filesystems to ensure that they do not
create a directory that has both xt_TCPMSS.h and xt_tcpmss.h at the
same time, for example, but the mechanism needs to allow visibility
into more than just a single path when the custom check is made
(e.g. a hook run in "write_index_as_tree()" can see all entries in
the index to make the decision; if we were to also hook into
"add_to_index()", the hook must be able to see other entries in the
index to which the new entry is being added).


[Footnote]

*1* "add_to_index()" could be another place to hook into, and doing
so has the merit of catching a breakage sooner, but I suspect that
alone may not be sufficient if there are other ways for new entries
to appear in the index.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

BUG: indent-with-non-tab always on (was: Re: [PATCH v6 00/12] Update git revisions)

[Marc Branchaud]

On 2016-08-15 11:00 AM, Philip Oakley wrote:

From: "Marc Branchaud" 

On 2016-08-12 07:45 PM, Philip Oakley wrote:

These are the quick fixes to Marc's comments to patches 5,6,11,
and a consequential change to 12.

Only the changed patches are included.


Looks good to me -- no further comments!

However, I still don't understand why git says 11/12 has "indent with
spaces" problems.


I'm guessing it's that the text is monospaced rather than tabbed, and
it's flagging that.

I'd noticed that it was highlighted in the Git gui (which I use to
visualise both the diff, the message and the part after the three dashes
at the same time).


Actually, it looks like an indent-with-non-tab problem, which is 
supposed to be off by default.


Git doesn't complain about the patch if I set
core.whitespace = tabwidth=11
presumably because the patch uses 10 spaces to indent the offending lines.

But explicitly disabling that check with
core.whitespace = -indent-with-non-tab
doesn't work.

Unfortunately, I don't have time today to track this down.

M.

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

Re: [PATCH 1/3] diff-highlight: add some tests.

[Lars Schneider]

> On 30 Jul 2016, at 17:11, Brian Henderson  wrote:
> 
> ---
> contrib/diff-highlight/Makefile  |  5 ++
> contrib/diff-highlight/t/Makefile| 19 +++
> contrib/diff-highlight/t/t9400-diff-highlight.sh | 63 ++
> contrib/diff-highlight/t/test-diff-highlight.sh  | 69 
> 4 files changed, 156 insertions(+)
> create mode 100644 contrib/diff-highlight/Makefile
> create mode 100644 contrib/diff-highlight/t/Makefile
> create mode 100644 contrib/diff-highlight/t/t9400-diff-highlight.sh
> create mode 100644 contrib/diff-highlight/t/test-diff-highlight.sh
> 
> diff --git a/contrib/diff-highlight/Makefile b/contrib/diff-highlight/Makefile

Would it make sense to add the contrib tests to the Travis-CI build, too?
https://travis-ci.org/git/git/branches

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

Re: [PATCH 1/3] diff-highlight: add some tests.

[Brian Henderson]

On Wed, Aug 10, 2016 at 08:56:35AM +, Eric Wong wrote:



> Typically, we expect a reroll in a few days, and I guess there's
> no rush (so you can squash your comment patch in addressing
> Junio's concern into 3/3).
> 
> Thanks.

thanks, (slowly) working on an update.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] diff-highlight: add some tests.

[Junio C Hamano]

Brian Henderson  writes:

> On Wed, Aug 10, 2016 at 08:56:35AM +, Eric Wong wrote:
>
> 
>
>> Typically, we expect a reroll in a few days, and I guess there's
>> no rush (so you can squash your comment patch in addressing
>> Junio's concern into 3/3).
>> 
>> Thanks.
>
> thanks, (slowly) working on an update.

Thanks, both, for keeping the ball rolling ;-)

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

Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix

[Junio C Hamano]

Johannes Schindelin  writes:

> On Sun, 14 Aug 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>> 
>> > -  test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g')
>> > +  test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')
>> ...
> I know that this is so because my first iteration of the patch did exactly
> what you suggested.

You probably should have stepped back and taken a deep breath before
writing the second round.  Doing so after writing it before sending
would also have been OK.

Among three characters that are special cased here, the problem "if
we squish a run of problematic characters into one underscore, we
risk making the result ambiguous" is NOT limited to '>'; it is not a
new problem with '>', either.  I can think of two other possible
solutions offhand:

 (1) drop "squishing a run", i.e. [/ ]*, from the regexp, and rename
 existing test vectors that would be affected;

 (2) change the string used in the offending test so that squishing
 will not make the result ambiguous.

before special casing "y/>/_/"; as there is nothing in ">" that
inherently causes the ambiguity that won't be caused by "/" or " ",
adding second clause to the sed expression that does things
differently only for ">" is simply wrong.  After all, you only
wanted to affect the "prefix=-->" test and not the whole set of the
tests in the script.

Obviously (1) is a lot of impact with little gain, and as Jacob
already offered to do, I think (2) is a lot more sensible solution
and it also is more in line with your "If it isn't broken, do not
fix it", I would say.


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

Re: [PATCH] t/Makefile: make sure that file names are truly platform-independent

[Junio C Hamano]

Johannes Schindelin  writes:

> Some file names that are okay on ext4 and on HFS+ are illegal in
> Windows. In order to stay truly platform-independent, Git's source code
> must not contain such illegal file names, even if things just happen to
> work on Linux.

Good thinking.

Some tests may have to be skipped on platforms that cannot express
certain paths, but even then they shouldn't ship a file with
pathname that cannot even be checked out (they should instead create
and use such a path, protected behind filesystem specific test
prerequisite).

> +test-lint-filenames:
> + @illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \

This pattern must exclude questionables on either NTFS or HFS+; it
is ironic that it is not even sufficient to limit ourselves to the
Portable Character Set [*1*], but such is life.

By the way, doesn't ls-files take pathspec glob, saving one extra
process to run grep?

master$ git ls-files '*["*:<>?\\|]*'
pu$ git ls-files '*["*:<>?\\|]*'
t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side

Thanks.

> + test -z "$$illegal" || { \
> + echo >&2 "illegal file name(s): " $$illegal; exit 1; }

[Reference]

*1* 
http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap06.html#tag_06_01
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [ANNOUNCE] Git v2.10.0-rc0

[Junio C Hamano]

Torsten Bögershausen  writes:

> On 15.08.16 00:47, Junio C Hamano wrote:
>> Torsten Bögershausen (1):
>>   convert: unify the "auto" handling of CRLF
>
> Should we mention this change in the release notes?
>
> The handling of "*.text = auto" was changed, and now
>
> $ echo "* text=auto eol=crlf" >.gitattributes
> has the same effect as
> $ git config core.autocrlf true

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

Re: [PATCH v1 3/3] doc: rebase: clarify fixup! fixup! constraint

[Junio C Hamano]

"Philip Oakley"  writes:

>> I think this is about
>
> Yes, but the original wording didn't make me think that.

Yeah, it is very plausible that it is not limited to you, and I
agree that it is worthwhile to update the description around here.

>>> Also, does 'earliest commit requiring fixup/squash' fully convey that
>>> its the one to fix.
>>
>> I cannot tell if that a question or a statement?
>
> It's a question. In your prior para you offer "they fix the very first
> commit that invited these fixups" as an alternate.

I think both are equally understandable to me (but I am not the
primary target audience).

> It's when a users mental model is that they got their first fixup
> wrong and it's that fixup they are correcting, and later they add
> different fixups to the orignal that it all gets hairy.
> (diffs must have the right sequence, while snapshots don't care - so
> if we keep the diff sequence, we don't care about the user's mental
> model as the end results are the same).
>
> The writeup needs to cope with the mental model rather than the end result.

Yes, yes.

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

Re: [PATCH v5 2/3] diff: add --diff-line-prefix option for passing in a prefix

[Junio C Hamano]

Jacob Keller  writes:

> I will look more into how to do the log version tomorrow, if I am
> still stuck I will re-work the patches as you suggest here.
>
> I am hoping I can find a good solution for how to handle it though.

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

[L10N] Kickoff of translation for Git 2.10.0 round 1

[Jiang Xin]

Hi,

Git v2.10.0-rc0 has been released, and it's time to start new round of git l10n.
This time there are 248 updated messages need to be translated since last
update:

l10n: git.pot: v2.10.0 round 1 (248 new, 56 removed)

Generate po/git.pot from v2.10.0-rc0 for git v2.10.0 l10n round 1.

Signed-off-by: Jiang Xin 

You can get it from the usual place:

https://github.com/git-l10n/git-po/

As how to update your XX.po and help to translate Git, please see
"Updating a XX.po file" and other sections in “po/README" file.

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

Re: [PATCH v6 00/12] Update git revisions

[Philip Oakley]

From: "Marc Branchaud" 

On 2016-08-12 07:45 PM, Philip Oakley wrote:

These are the quick fixes to Marc's comments to patches 5,6,11,
and a consequential change to 12.

Only the changed patches are included.


Looks good to me -- no further comments!

However, I still don't understand why git says 11/12 has "indent with 
spaces" problems.


I'm guessing it's that the text is monospaced rather than tabbed, and it's 
flagging that.


I'd noticed that it was highlighted in the Git gui (which I use to visualise 
both the diff, the message and the part after the three dashes at the same 
time).





M.



Philip Oakley (12):
  doc: use 'symmetric difference' consistently
  doc: revisions - name the left and right sides
  doc: show the actual left, right, and boundary marks
  doc: revisions: give headings for the two and three dot notations
  doc: revisions: extra clarification of ^! notation effects
  doc: revisions: single vs multi-parent notation comparison
  doc: gitrevisions - use 'reachable' in page description
  doc: gitrevisions - clarify 'latter case' is revision walk
  doc: revisions  - define `reachable`
  doc: revisions - clarify reachability examples
  doc: revisions: show revision expansion in examples
  doc: revisions: sort examples and fix alignment of the unchanged

 Documentation/gitk.txt |   2 +-
 Documentation/gitrevisions.txt |   6 +-
 Documentation/pretty-formats.txt   |   2 +-
 Documentation/rev-list-options.txt |   4 +-
 Documentation/revisions.txt| 125 
-

 5 files changed, 88 insertions(+), 51 deletions(-)


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

Re: [PATCH v6 00/12] Update git revisions

[Marc Branchaud]

On 2016-08-12 07:45 PM, Philip Oakley wrote:

These are the quick fixes to Marc's comments to patches 5,6,11,
and a consequential change to 12.

Only the changed patches are included.


Looks good to me -- no further comments!

However, I still don't understand why git says 11/12 has "indent with 
spaces" problems.


M.



Philip Oakley (12):
  doc: use 'symmetric difference' consistently
  doc: revisions - name the left and right sides
  doc: show the actual left, right, and boundary marks
  doc: revisions: give headings for the two and three dot notations
  doc: revisions: extra clarification of ^! notation effects
  doc: revisions: single vs multi-parent notation comparison
  doc: gitrevisions - use 'reachable' in page description
  doc: gitrevisions - clarify 'latter case' is revision walk
  doc: revisions  - define `reachable`
  doc: revisions - clarify reachability examples
  doc: revisions: show revision expansion in examples
  doc: revisions: sort examples and fix alignment of the unchanged

 Documentation/gitk.txt |   2 +-
 Documentation/gitrevisions.txt |   6 +-
 Documentation/pretty-formats.txt   |   2 +-
 Documentation/rev-list-options.txt |   4 +-
 Documentation/revisions.txt| 125 -
 5 files changed, 88 insertions(+), 51 deletions(-)


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

[PATCH] t/Makefile: make sure that file names are truly platform-independent

[Johannes Schindelin]

Some file names that are okay on ext4 and on HFS+ are illegal in
Windows. In order to stay truly platform-independent, Git's source code
must not contain such illegal file names, even if things just happen to
work on Linux.

One such file name was recently introduced into Git's `pu` branch:

t4013/diff.diff_--diff-line-prefix=-->_master_master^_sidt4013/diff.diff_--diff-line-prefix=-->_master_master^_sid

It is illegal because it contains the '>' character that is not part of
a valid filename on Windows.

To catch these issues early, let's introduce a new test-lint-* goal that
fails if such file names were detected and run it as part of a `make
test`.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/test-lint-filenames-v1
Fetch-It-Via: git fetch https://github.com/dscho/git test-lint-filenames-v1

This is the more complete fix I talked about.

 t/Makefile | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/t/Makefile b/t/Makefile
index 18e2b28..3c0eb48 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -52,7 +52,8 @@ clean-except-prove-cache:
 clean: clean-except-prove-cache
$(RM) .prove
 
-test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax
+test-lint: test-lint-duplicates test-lint-executable test-lint-shell-syntax \
+   test-lint-filenames
 
 test-lint-duplicates:
@dups=`echo $(T) | tr ' ' '\n' | sed 's/-.*//' | sort | uniq -d` && \
@@ -67,6 +68,11 @@ test-lint-executable:
 test-lint-shell-syntax:
@'$(PERL_PATH_SQ)' check-non-portable-shell.pl $(T) $(THELPERS)
 
+test-lint-filenames:
+   @illegal="$$(git ls-files | grep '["*:<>?\\|]')"; \
+   test -z "$$illegal" || { \
+   echo >&2 "illegal file name(s): " $$illegal; exit 1; }
+
 aggregate-results-and-cleanup: $(T)
$(MAKE) aggregate-results
$(MAKE) clean
-- 
2.9.2.691.g78954f3

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

Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix

[Johannes Schindelin]

Hi Junio,

On Sun, 14 Aug 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > -   test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g')
> > +   test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')
> 
> The existing sed scriptlet says "we cannot have slash and do not
> want to have space in filename, so we squash runs of them to a
> single underscore".  If you have more characters that you do not
> want, you should add that to the existing set instead.

No, we cannot do that. I even mentioned it in my commit message why:

We have to take particular care not to confound the existing
conversion of unwanted characters to underscores with the new
substitution of '>': the existing conversion chose to collapse
runs of multiple unwanted characters into a single underscore. If
we allowed '>' to be collapsed, too, the file name generated from
the command "diff [...]=-- [...]" would be identical to the one
generated from "diff [...]=--> [...]".

And as there is exactly this case (two command-lines, differing only in
the '>' character), doing what you suggested would *break* the test since
it would now look at the wrong file.

I know that this is so because my first iteration of the patch did exactly
what you suggested.

> While you are at it, it may be sensible to add a colon to that set, too,
> no?
> 
> Something like this, perhaps?
> 
> > -   test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g')
> > +   test=$(echo "$cmd" | sed -e 's|[/ <:>][/ <:>]*|_|g')

Maybe. But what other characters are missing? Those are not the only ones
illegal on Windows: [/ \0-\x1f"*:<>?\\|] would be a more complete version
of what you suggested. Except that it is not a valid basic regex.

But is that really necessary?

I think not, for the following reasons:

- my patch was specifically designed to stop my CI from pestering me about
  a totally broken revision that cannot even be checked out (and causes
  subsequent problems because of it),

- as such, my patch was meant not to be an all-encompassing solution to
  the problem of filenames that are illegal on Windows, but really a tiny
  patch that you could apply as quickly as possible so that my CI jobs
  would stop pestering me (which they did not, because `pu` is still
  broken),

- I even meant this little patch to be so small that it can be easily
  squashed into Jacob's patch,

- I do not want to complicate regular expressions unless *really* needed
  (and you have to admit that we do not need to address any more
  characters than the '>' *right now*), as

- regular expressions are not exactly an easy meal, so it makes
  sense to keep them as simple as possible both for contributors'
  as well as for maintainers' sake, and

- when trying to come up with a super-complete solution, it is
  really easy not only to spend way too much time on it, but
  also to introduce bugs that are not spotted for a lng time
  because nothing actually exercises the newly introduced code, and

- If It Ain't Broke, Don't Fix It.

- the broader solution must come separately, and not as a mere add-on to
  one test case: we really need to ensure that such file names do not
  enter Git's source code.

I will send my proposal to address the larger issue in a moment, in the
meantime I *beg* you to add this here patch as a quick fix to my CI woes,
either by squashing it into the indicated commit, or by appending it to
the topic branch. I do not care which one, as long as `pu` gets fixed.

Thanks,
Dscho


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

Re: [ANNOUNCE] Git v2.10.0-rc0

[Torsten Bögershausen]

On 15.08.16 00:47, Junio C Hamano wrote:
> Torsten Bögershausen (1):
>   convert: unify the "auto" handling of CRLF

Should we mention this change in the release notes?

The handling of "*.text = auto" was changed, and now

$ echo "* text=auto eol=crlf" >.gitattributes
has the same effect as
$ git config core.autocrlf true


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

Re: storing cover letter of a patch series?

[John Keeping]

On Mon, Aug 15, 2016 at 08:30:03PM +0700, Duy Nguyen wrote:
> On Mon, Aug 15, 2016 at 7:37 PM, Philip Oakley  wrote:
> > I appreciate there has been a lot of discussion, but it mainly appears to be
> > about an upstream / integration viewpoint.
> >
> > I'd hate it if there was a one size fits all solution that was only focused
> > on one important use case, rather than having at least a simple fallback for
> > simple folk.
> >
> > Personally I liked the idea that I could start my patch series branch with a
> > simple 'empty' commit with a commit message that read "cover!  > the series>" and continue with the cover letter. It's essentially the same
> > as the fixup! and squash! idea (more the latter - it's squash! without a
> > predecessor). For moderate size series a simple 'git rebase master..' is
> > sufficient to see the whole series and decide which need editing, rewording,
> > swapping, checking the fixups, etc.
> 
> I think you hit the jackpot (or are getting very close). This removes
> the special status of "the commit at the tip of the branch" cover
> letter. Maybe I just like it so much I have a hard time finding
> anything wrong with it :)

I haven't followed this thread too closely, but has anyone mentioned
U-Boot's patman tool[1] yet?

It defines several special trailers that can be used to annotate commits
with additional information to use when mailing them and which are
automatically removed from the commit message in patches sent using
patman.


[1] http://git.denx.de/?p=u-boot.git;a=blob;f=tools/patman/README
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: storing cover letter of a patch series?

[Duy Nguyen]

On Mon, Aug 15, 2016 at 7:37 PM, Philip Oakley  wrote:
> I appreciate there has been a lot of discussion, but it mainly appears to be
> about an upstream / integration viewpoint.
>
> I'd hate it if there was a one size fits all solution that was only focused
> on one important use case, rather than having at least a simple fallback for
> simple folk.
>
> Personally I liked the idea that I could start my patch series branch with a
> simple 'empty' commit with a commit message that read "cover!  the series>" and continue with the cover letter. It's essentially the same
> as the fixup! and squash! idea (more the latter - it's squash! without a
> predecessor). For moderate size series a simple 'git rebase master..' is
> sufficient to see the whole series and decide which need editing, rewording,
> swapping, checking the fixups, etc.

I think you hit the jackpot (or are getting very close). This removes
the special status of "the commit at the tip of the branch" cover
letter. Maybe I just like it so much I have a hard time finding
anything wrong with it :)

> Format-patch would then be taught to spot that the first commit in the
> series is "cover! " and create the usual 0/N cover letter. Git Gui
> may need to be taught to recognise cover! (haven't checked if it recognises
> an empty commit squash!). Possibly 'git commit' may want a --cover option to
> massage the commit message and add --allow-empty, but that's finesse.
>
> I've no problem with more extensive methods for those preparing very big
> patch series, or with those needing to merge together a lot of series and
> want to keep the cover letters, but ensuring that a simple flow is possible
> should still be there.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git shallow clone branch doesn't work with recursive submodules cloning

[Jeff King]

On Mon, Aug 15, 2016 at 03:53:48PM +0300, Arkady Shapkin wrote:

> So it will work only if github update their server configuration
> (boringssl submodule on github)?

Correct.

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

Re: Git shallow clone branch doesn't work with recursive submodules cloning

[Arkady Shapkin]

So it will work only if github update their server configuration
(boringssl submodule on github)?

2016-08-15 15:47 GMT+03:00 Jeff King :
> On Mon, Aug 15, 2016 at 03:29:14PM +0300, Arkady Shapkin wrote:
>
>> Thank you, after updating to "2.9.3.windows.1" options "--recursive
>> --depth 1" now works.
>>
>> But "--recursive --shallow-submodules" and "--recursive
>> --shallow-submodules --depth 1" still doesn't work.
>
> It does "work", but the server hosting your submodule may not be
> configured to allow you to access the reachable-but-non-tip sha1
> directly. So it's not a bug, but rather a configuration issue (and the
> "fix" in v2.9.1 is to be less aggressive about enabling
> shallow-submodules, since the default server configuration does not
> allow it to work well).
>
> More discussion is in:
>
>   
> http://public-inbox.org/git/ofe09d48f2.d1d14f49-onc2257fd7.00280736-c2257fd7.00282...@notes.na.collabserv.com/t/#u
>
> -Peff



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

Re: [PATCH] rev-parse: respect core.hooksPath in --git-path

[Jeff King]

On Mon, Aug 15, 2016 at 02:43:18PM +0200, Johannes Schindelin wrote:

> The idea of the --git-path option is not only to avoid having to
> prefix paths with the output of --git-dir all the time, but also to
> respect overrides for specific common paths inside the .git directory
> (e.g. `git rev-parse --git-path objects` will report the value of
> the environment variable GIT_OBJECT_DIRECTORY, if set).
> 
> When introducing the core.hooksPath setting, we forgot to adjust
> git_path() accordingly. This patch fixes that.

Makes sense.

I think you can squash in:

diff --git a/run-command.c b/run-command.c
index 33bc63a..5a4dbb6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -824,10 +824,7 @@ const char *find_hook(const char *name)
static struct strbuf path = STRBUF_INIT;
 
strbuf_reset();
-   if (git_hooks_path)
-   strbuf_addf(, "%s/%s", git_hooks_path, name);
-   else
-   strbuf_git_path(, "hooks/%s", name);
+   strbuf_git_path(, "hooks/%s", name);
if (access(path.buf, X_OK) < 0)
return NULL;
return path.buf;

as strbuf_git_path() handles this now.

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

Re: Git shallow clone branch doesn't work with recursive submodules cloning

[Jeff King]

On Mon, Aug 15, 2016 at 03:29:14PM +0300, Arkady Shapkin wrote:

> Thank you, after updating to "2.9.3.windows.1" options "--recursive
> --depth 1" now works.
> 
> But "--recursive --shallow-submodules" and "--recursive
> --shallow-submodules --depth 1" still doesn't work.

It does "work", but the server hosting your submodule may not be
configured to allow you to access the reachable-but-non-tip sha1
directly. So it's not a bug, but rather a configuration issue (and the
"fix" in v2.9.1 is to be less aggressive about enabling
shallow-submodules, since the default server configuration does not
allow it to work well).

More discussion is in:

  
http://public-inbox.org/git/ofe09d48f2.d1d14f49-onc2257fd7.00280736-c2257fd7.00282...@notes.na.collabserv.com/t/#u

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

[PATCH] rev-parse: respect core.hooksPath in --git-path

[Johannes Schindelin]

The idea of the --git-path option is not only to avoid having to
prefix paths with the output of --git-dir all the time, but also to
respect overrides for specific common paths inside the .git directory
(e.g. `git rev-parse --git-path objects` will report the value of
the environment variable GIT_OBJECT_DIRECTORY, if set).

When introducing the core.hooksPath setting, we forgot to adjust
git_path() accordingly. This patch fixes that.

Signed-off-by: Johannes Schindelin 
---
Published-As: https://github.com/dscho/git/releases/tag/git-path-hooks-v1
Fetch-It-Via: git fetch https://github.com/dscho/git git-path-hooks-v1

 path.c   | 2 ++
 t/t1350-config-hooks-path.sh | 6 ++
 2 files changed, 8 insertions(+)

diff --git a/path.c b/path.c
index 17551c4..fe3c4d9 100644
--- a/path.c
+++ b/path.c
@@ -380,6 +380,8 @@ static void adjust_git_path(struct strbuf *buf, int 
git_dir_len)
  get_index_file(), strlen(get_index_file()));
else if (git_db_env && dir_prefix(base, "objects"))
replace_dir(buf, git_dir_len + 7, get_object_directory());
+   else if (git_hooks_path && dir_prefix(base, "hooks"))
+   replace_dir(buf, git_dir_len + 5, git_hooks_path);
else if (git_common_dir_env)
update_common_dir(buf, git_dir_len, NULL);
 }
diff --git a/t/t1350-config-hooks-path.sh b/t/t1350-config-hooks-path.sh
index 5e3fb3a..f1f9aee 100755
--- a/t/t1350-config-hooks-path.sh
+++ b/t/t1350-config-hooks-path.sh
@@ -34,4 +34,10 @@ test_expect_success 'Check that various forms of specifying 
core.hooksPath work'
test_cmp expect actual
 '
 
+test_expect_success 'git rev-parse --git-path hooks' '
+   git config core.hooksPath .git/custom-hooks &&
+   git rev-parse --git-path hooks/abc >actual &&
+   test .git/custom-hooks/abc = "$(cat actual)"
+'
+
 test_done
-- 
2.9.2.691.g78954f3

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

Re: storing cover letter of a patch series?

[Philip Oakley]

From: "Duy Nguyen" 

On Mon, Aug 15, 2016 at 1:28 PM, Stefan Beller  wrote:

On Sun, Aug 14, 2016 at 12:15 AM, Jacob Keller 
wrote:

On Sat, Aug 13, 2016 at 1:49 AM, Duy Nguyen  wrote:

On Tue, Aug 9, 2016 at 12:27 AM, Stefan Beller 
wrote:

is what you want. Maybe we want to see a patch that adds the reverse
functionality as well, i.e. git-am will store the the cover letter as
the
branch description and git-merge will propose the branch description
for
the merge commit.


I almost suggested the same, but there is a problem with this
approach: if you're are on a detached head, where does git-am save it?


What would the user expect? We can have a range of expectations:
1) reject and error out git-am
2) warn about not saving branch.description and continue with am
3) have a (maybe special) branch.HEAD.description thing, same for
FETCH_HEAD etc
4) have a config option to choose between 1 and 2, if unset default to 1

I think 3 is a bad choice.
4 seems reasonable to me, though I wonder if some people use git-am in
a scripted workflow with a detached head and then create the branch
afterwards?
So

5) create a branch for them? (such as $(date)-${subject})

My gut reaction doesn't like 5 either.


I'm starting to think option 6 (storing cover latter as an empty
commit at tip then git-merge replaces it with a merge commit in a
permanent history) may be the way to go. It handles detached heads
just fine, we have reflog to store older cover letters. Though it will
not play nice with 'git commit --amend' and 'git reset' for people who
rewrites history heavily during development, but maybe 'git rebase -i
--autosquash' would be an ok workflow alternative.
--


[sorry if this is not the right place to 'drop in'..]
I appreciate there has been a lot of discussion, but it mainly appears to be
about an upstream / integration viewpoint.

I'd hate it if there was a one size fits all solution that was only focused
on one important use case, rather than having at least a simple fallback for
simple folk.

Personally I liked the idea that I could start my patch series branch with a
simple 'empty' commit with a commit message that read "cover! " and continue with the cover letter. It's essentially the same
as the fixup! and squash! idea (more the latter - it's squash! without a
predecessor). For moderate size series a simple 'git rebase master..' is
sufficient to see the whole series and decide which need editing, rewording,
swapping, checking the fixups, etc.

Format-patch would then be taught to spot that the first commit in the
series is "cover! " and create the usual 0/N cover letter. Git Gui
may need to be taught to recognise cover! (haven't checked if it recognises
an empty commit squash!). Possibly 'git commit' may want a --cover option to
massage the commit message and add --allow-empty, but that's finesse.

I've no problem with more extensive methods for those preparing very big
patch series, or with those needing to merge together a lot of series and
want to keep the cover letters, but ensuring that a simple flow is possible
should still be there.
--
Philip

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

Re: Git shallow clone branch doesn't work with recursive submodules cloning

[Arkady Shapkin]

Thank you, after updating to "2.9.3.windows.1" options "--recursive
--depth 1" now works.

But "--recursive --shallow-submodules" and "--recursive
--shallow-submodules --depth 1" still doesn't work.

2016-08-15 15:04 GMT+03:00 Jeff King :
> On Mon, Aug 15, 2016 at 02:20:27PM +0300, Arkady Shapkin wrote:
>
>> I am trying clone repository by tag with recursive submodules init,
>> but for one submodule it doesn't work.
>> What I'm doing wrong?
>
> Nothing. See 18a74a0 (clone: do not let --depth imply
> --shallow-submodules, 2016-06-19).
>
>> >git --version
>> git version 2.9.0.windows.1
>
> The fix is in v2.9.1.
>
> -Peff



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

Re: [PATCH] make rebase respect core.hooksPath if set

[Jeff King]

On Mon, Aug 15, 2016 at 02:24:59PM +0200, Johannes Schindelin wrote:

> > when looking for pre-rebase and post-rewrite hooks, git-rebase
> > only looks for hooks dir using `git rev-parse --git-path hooks`,
> > which didn't consider the path overridden by core.hooksPath.
> 
> Would it not be more appropriate to teach --git-path hooks to respect the
> core.hooksPath variable? This would be in line with --git-path objects
> respecting the GIT_OBJECT_DIRECTORY environment variable.

Good idea. I think that logic is all in path.c:adjust_git_path().

And then I suspect the manual handling of git_hooks_path in find_hook()
could go away (because strbuf_git_path would just do the right thing
automatically).

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

Re: [PATCH 1/1] do not add common-main to lib

[Christian Hesse]

Jeff King  on Mon, 2016/08/15 08:02:
> On Mon, Aug 15, 2016 at 09:52:07AM +0200, Christian Hesse wrote:
> 
> > From: Christian Hesse 
> > 
> > Commit 08aade70 (mingw: declare main()'s argv as const) changed
> > declaration of main function. This breaks linking external projects
> > (e.g. cgit) to libgit.a with:
> > 
> > error: Multiple definition of `main'  
> 
> I'd expect the culprit is actually 3f2e229 (add an extra level of
> indirection to main(), 2016-07-01).

Ah, probably you are right...

> > So do not add common-main to lib and let projects have their own
> > main function.  
> 
> That is certainly an option, but I think it means that those projects
> are potentially buggy in the same way that some git commands were prior
> to the common-main series. Namely, the common main() may do some
> run-time setup that parts of libgit.a assume has been done.

Ok, got it.

> I would not be surprised if cgit crashes on Windows, for instance, for
> the reasons detailed in 650c449 (common-main: call
> git_extract_argv0_path(), 2016-07-01). I would also not be surprised if
> nobody actually builds cgit on Windows. :)

I never tried and probably nobody else did. :-p

> The "right" way to do it (according to the way libgit.a views the world)
> is for cgit's main to become cmd_main(), and let libgit.a do its
> run-time startup before getting there.

Looks like that does the job. I will give it some more testing.

Please ignore my patch... ;)
Thanks a lot!
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgpptHBMuwcrB.pgp
Description: OpenPGP digital signature

Re: [PATCH 1/1] do not add common-main to lib

[Christian Hesse]

Johannes Schindelin  on Mon, 2016/08/15 14:20:
> Hi Christian,
> 
> On Mon, 15 Aug 2016, Christian Hesse wrote:
> 
> > From: Christian Hesse 
> > 
> > Commit 08aade70 (mingw: declare main()'s argv as const) changed
> > declaration of main function. This breaks linking external projects
> > (e.g. cgit) to libgit.a with:
> > 
> > error: Multiple definition of `main'
> > 
> > So do not add common-main to lib and let projects have their own
> > main function.  
> 
> I am opposed to this change.

Me too. :-p

> For one, libgit.a is *not* a library with an API, for a good reason:
> nothing in Git's development guarantees any kind of stable API. For that
> reason, libgit.a is not installed, either, and neither are any headers.
> 
> And even more importantly: *iff* you *insist* on using libgit.a in your
> project *despite* having been told not to, it is your responsibility to
> stay up-to-date with the requirements of it.

cgit pulls in the git tree as a subproject. We are aware that the API changes
all the time and that's fine. Usually we just fix it, this time I missed the
background information of the change.

> One such requirement is that you now implement cmd_main() instead of
> main().
> 
> So if you want to continue to have an out-of-tree project that links
> against the (private) libgit.a, it is your out-of-tree project that needs
> changing, not libgit.a.

Already updated my code. ;)
Thanks!
-- 
main(a){char*c=/*Schoene Gruesse */"B?IJj;MEH"
"CX:;",b;for(a/*Best regards my address:*/=0;b=c[a++];)
putchar(b-1/(/*Chriscc -ox -xc - && ./x*/b/42*2-3)*42);}


pgp1A1xIdqYyY.pgp
Description: OpenPGP digital signature

Re: Getting the "message" given a branch designation and conversely

[Jeff King]

On Sun, Aug 14, 2016 at 07:58:14AM -0700, n...@dad.org wrote:

> I am learning how to use git. I would like to know how:
> 
> Given a branch's designation, such as "master~4", how can I see the message I
> furnished when I created the branch using "git commit"?

Somebody already pointed you at "git log", which is the right tool for
looking at commit messages (or perhaps "git show" if you only want to
see a single entry).

> Conversely, given the message I furnished to "git commit", when I created a
> branch, how can I see the branch's designation?

Try "git log --grep=some.regex" to find a particular commit. Usually we
refer to commits by their sha1 id, which will be shown by git-log.
However, you can use git-describe to generate a name for any commit that
is based on traversing from a tag. Try:

  git describe --contains --all 

for example. Using "--all" tells git to consider names based on branches
as well as tags. Using "--contains" will generate a name based on
traversing backwards from the tags and branches (like "master~4") rather
than basing the name on a tag that you build off of.

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