Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block

[Jonathan Tan]

On 10/17/2016 06:42 PM, Junio C Hamano wrote:

Stefan Beller  writes:


On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  wrote:


 Existing trailers are extracted from the input message by looking for
-a group of one or more lines that contain a colon (by default), where
+a group of one or more lines in which at least one line contains a
+colon (by default), where


Please see commit
578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
for an example where I think this is overly broad.


Hmph.  That's a merge.

Merge branch 'rs/c-auto-resets-attributes'

When "%C(auto)" appears at the very beginning of the pretty format
string, it did not need to issue the reset sequence, but it did.

* rs/c-auto-resets-attributes:
  pretty: avoid adding reset for %C(auto) if output is empty

And neither of the two colon containing line remotely resembles how
a typical RFC-822 header is formatted.  So that may serve as a hint
to how we can tighten it without introducing false negative.


The only "offending" character is the space (according to RFC 822), but 
that sounds like a good rule to have.



Another made up example, that I'd want to feed
in commit -s eventually:

--8<--
demonstrate colons in Java

First paragraph is not interesting.

Also if using another Language such as Java, where I point out
Class::function() to be problematic
--8<--

This would lack the white space between the last paragraph and
the Sign off ?

So for this patch I am mostly concerned about false positives hidden
in actual text.


Yes.

These are exactly why I mentioned "if certian number or percentage"
in my earlier suggestion.

I think in practice, "A paragraph with at least one Signed-off-by:
line, and has no more than 3/4 of the (logical) lines that do not
resemble how a typical RFC-822 header is formatted" or something
along that line would give us a reasonable safety.


I think that "Signed-off-by:" is not guaranteed to be present. Defining 
a trailer line as "a line starting with a token, then optional 
whitespace, then separator", maybe the following rule:
- at least one trailer line generated by Git ("(cherry picked by" or 
"Signed-off-by") or configured in the "trailer" section in gitconfig

OR
- at least 3/4 logical trailer lines (I'm wondering if this should be 
100% trailer lines)


?


Your Java example will fail the criteria in two ways, so we'd be
safe ;-)

Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block

[Junio C Hamano]

Stefan Beller  writes:

> On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  
> wrote:
>>
>>  Existing trailers are extracted from the input message by looking for
>> -a group of one or more lines that contain a colon (by default), where
>> +a group of one or more lines in which at least one line contains a
>> +colon (by default), where
>
> Please see commit
> 578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
> for an example where I think this is overly broad.

Hmph.  That's a merge.

Merge branch 'rs/c-auto-resets-attributes'

When "%C(auto)" appears at the very beginning of the pretty format
string, it did not need to issue the reset sequence, but it did.

* rs/c-auto-resets-attributes:
  pretty: avoid adding reset for %C(auto) if output is empty

And neither of the two colon containing line remotely resembles how
a typical RFC-822 header is formatted.  So that may serve as a hint
to how we can tighten it without introducing false negative.

> Another made up example, that I'd want to feed
> in commit -s eventually:
>
> --8<--
> demonstrate colons in Java
>
> First paragraph is not interesting.
>
> Also if using another Language such as Java, where I point out
> Class::function() to be problematic
> --8<--
>
> This would lack the white space between the last paragraph and
> the Sign off ?
>
> So for this patch I am mostly concerned about false positives hidden
> in actual text.

Yes.  

These are exactly why I mentioned "if certian number or percentage"
in my earlier suggestion.

I think in practice, "A paragraph with at least one Signed-off-by:
line, and has no more than 3/4 of the (logical) lines that do not
resemble how a typical RFC-822 header is formatted" or something
along that line would give us a reasonable safety.  

Your Java example will fail the criteria in two ways, so we'd be
safe ;-)

Re: [PATCH v3 6/6] trailer: support values folded to multiple lines

[Stefan Beller]

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  wrote:
> Currently, interpret-trailers requires that a trailer be only on 1 line.
> For example:
>
>   a: first line
>  second line
>
> would be interpreted as one trailer line followed by one non-trailer line.
>
> Make interpret-trailers support RFC 822-style folding, treating those
> lines as one logical trailer.
>
> Signed-off-by: Jonathan Tan 
> ---

Looks good,

Thanks,
Stefan

Re: [PATCH v3 5/6] trailer: allow non-trailers in trailer block

[Stefan Beller]

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  wrote:
>
>  Existing trailers are extracted from the input message by looking for
> -a group of one or more lines that contain a colon (by default), where
> +a group of one or more lines in which at least one line contains a
> +colon (by default), where

Please see commit
578e6021c0819d7be1179e05e7ce0e6fdb2a01b7
for an example where I think this is overly broad.

Another made up example, that I'd want to feed
in commit -s eventually:

--8<--
demonstrate colons in Java

First paragraph is not interesting.

Also if using another Language such as Java, where I point out
Class::function() to be problematic
--8<--

This would lack the white space between the last paragraph and
the Sign off ?

So for this patch I am mostly concerned about false positives hidden
in actual text.

Re: [PATCH v3 4/6] trailer: make args have their own struct

[Stefan Beller]

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  wrote:
> Improve type safety by making arguments (whether from configuration or
> from the command line) have their own "struct arg_item" type, separate
> from the "struct trailer_item" type used to represent the trailers in
> the buffer being manipulated.
>
> This change also prepares "struct trailer_item" to be further
> differentiated from "struct arg_item" in future patches.
>
> Signed-off-by: Jonathan Tan 
> ---
>  trailer.c | 135 
> +++---
>  1 file changed, 85 insertions(+), 50 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index 54cc930..a9ed3f8 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -29,6 +29,12 @@ struct trailer_item {
> struct list_head list;
> char *token;
> char *value;
> +};
> +
> +struct arg_item {
> +   struct list_head list;
> +   char *token;
> +   char *value;
> struct conf_info conf;
>  };

(Unrelated side note:) When first seeing this diff, I assumed the diff
heuristic is going wild, because it doesn't add a full struct.
But on a second closer look, I realize this is the only correct diff,
because we do not account for moved lines from one struct to the
other.


>  static void add_arg_to_input_list(struct trailer_item *on_tok,
> - struct trailer_item *arg_tok)
> + struct arg_item *arg_tok)
>  {
> -   if (after_or_end(arg_tok->conf.where))
> -   list_add(_tok->list, _tok->list);
> +   int aoe = after_or_end(arg_tok->conf.where);
> +   struct trailer_item *to_add = trailer_from_arg(arg_tok);
> +   if (aoe)

The use of an extra variable here is more readable than my
imagined version of inlining to_add into the list_add calls
just to save aoe.

Looks good to me.

Stefan

Re: [PATCH v3 3/6] trailer: streamline trailer item create and add

[Stefan Beller]

On Fri, Oct 14, 2016 at 10:38 AM, Jonathan Tan  wrote:
> Currently, creation and addition (to a list) of trailer items are spread
> across multiple functions. Streamline this by only having 2 functions:
> one to parse the user-supplied string, and one to add the parsed
> information to a list.
>
> Signed-off-by: Jonathan Tan 

Reviewed-by: Stefan Beller 

Re: [PATCH] submodule--helper: normalize funny urls

[Stefan Beller]

On Mon, Oct 17, 2016 at 3:49 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> +static void strip_url_ending(char *url, size_t *_len)
>> +{
>> + int check_url_stripping = 1;
>> + size_t len = _len ? *_len : strlen(url);
>> +
>> + while (check_url_stripping) {
>> + check_url_stripping = 0;
>> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {
>
> This is "strip /. at the end" it seems.
>
> Does anything in the loop control guarantees 2 <= len at this point?

Oh, thanks for pointing that out. I thought about that and missed to add it.
I'll reroll with the length check once we hear back from Windows folks,
that this is a viable strategy for them, too.

Thanks,
Stefan

>
>> + url[len-2] = '\0';
>> + len -= 2;
>> + check_url_stripping = 1;
>> + }
>> +
>> + if (is_dir_sep(url[len-1])) {
>
> This is "strip / at the end" it seems.
>
> Does anything in the loop control guarantees 1 <= len at this point?
>
>> + url[len-1] = '\0';
>> + len--;
>> + check_url_stripping = 1;
>> + }
>> + }

Re: [PATCH v3 1/6] trailer: improve const correctness

[Stefan Beller]

On Fri, Oct 14, 2016 at 10:37 AM, Jonathan Tan  wrote:
> Change "const char *" to "char *" in struct trailer_item and in the
> return value of apply_command (since those strings are owned strings).
>
> Change "struct conf_info *" to "const struct conf_info *" (since that
> struct is not modified).
>
> Signed-off-by: Jonathan Tan 

Reviewed-by Stefan Beller 

> ---
>  trailer.c | 14 +++---
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index c6ea9ac..1f191b2 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -27,8 +27,8 @@ static struct conf_info default_conf_info;
>  struct trailer_item {
> struct trailer_item *previous;
> struct trailer_item *next;
> -   const char *token;
> -   const char *value;
> +   char *token;
> +   char *value;
> struct conf_info conf;
>  };
>
> @@ -95,8 +95,8 @@ static void free_trailer_item(struct trailer_item *item)
> free(item->conf.name);
> free(item->conf.key);
> free(item->conf.command);
> -   free((char *)item->token);
> -   free((char *)item->value);
> +   free(item->token);
> +   free(item->value);
> free(item);
>  }
>
> @@ -215,13 +215,13 @@ static struct trailer_item *remove_first(struct 
> trailer_item **first)
> return item;
>  }
>
> -static const char *apply_command(const char *command, const char *arg)
> +static char *apply_command(const char *command, const char *arg)
>  {
> struct strbuf cmd = STRBUF_INIT;
> struct strbuf buf = STRBUF_INIT;
> struct child_process cp = CHILD_PROCESS_INIT;
> const char *argv[] = {NULL, NULL};
> -   const char *result;
> +   char *result;
>
> strbuf_addstr(, command);
> if (arg)
> @@ -425,7 +425,7 @@ static int set_if_missing(struct conf_info *item, const 
> char *value)
> return 0;
>  }
>
> -static void duplicate_conf(struct conf_info *dst, struct conf_info *src)
> +static void duplicate_conf(struct conf_info *dst, const struct conf_info 
> *src)
>  {
> *dst = *src;
> if (src->name)
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [PATCH] submodule--helper: normalize funny urls

[Junio C Hamano]

Stefan Beller  writes:

> +static void strip_url_ending(char *url, size_t *_len)
> +{
> + int check_url_stripping = 1;
> + size_t len = _len ? *_len : strlen(url);
> +
> + while (check_url_stripping) {
> + check_url_stripping = 0;
> + if (is_dir_sep(url[len-2]) && url[len-1] == '.') {

This is "strip /. at the end" it seems.

Does anything in the loop control guarantees 2 <= len at this point?

> + url[len-2] = '\0';
> + len -= 2;
> + check_url_stripping = 1;
> + }
> +
> + if (is_dir_sep(url[len-1])) {

This is "strip / at the end" it seems.

Does anything in the loop control guarantees 1 <= len at this point?

> + url[len-1] = '\0';
> + len--;
> + check_url_stripping = 1;
> + }
> + }

Re: What's cooking in git.git (Oct 2016, #04; Mon, 17)

[Pranit Bauva]

Hey Junio,

On Tue, Oct 18, 2016 at 3:58 AM, Junio C Hamano  wrote:
> * pb/bisect (2016-10-14) 28 commits
>  - SQUASH???
>  - bisect--helper: remove the dequote in bisect_start()
>  - bisect--helper: retire `--bisect-auto-next` subcommand
>  - bisect--helper: retire `--bisect-autostart` subcommand
>  - bisect--helper: retire `--bisect-write` subcommand
>  - bisect--helper: `bisect_replay` shell function in C
>  - bisect--helper: `bisect_log` shell function in C
>  - bisect--helper: retire `--write-terms` subcommand
>  - bisect--helper: retire `--check-expected-revs` subcommand
>  - bisect--helper: `bisect_state` & `bisect_head` shell function in C
>  - bisect--helper: `bisect_autostart` shell function in C
>  - bisect--helper: retire `--next-all` subcommand
>  - bisect--helper: retire `--bisect-clean-state` subcommand
>  - bisect--helper: `bisect_next` and `bisect_auto_next` shell function in C
>  - t6030: no cleanup with bad merge base
>  - bisect--helper: `bisect_start` shell function partially in C
>  - bisect--helper: `get_terms` & `bisect_terms` shell function in C
>  - bisect--helper: `bisect_next_check` & bisect_voc shell function in C
>  - bisect--helper: `check_and_set_terms` shell function in C
>  - bisect--helper: `bisect_write` shell function in C
>  - bisect--helper: `is_expected_rev` & `check_expected_revs` shell function 
> in C
>  - bisect--helper: `bisect_reset` shell function in C
>  - wrapper: move is_empty_file() and rename it as is_empty_or_missing_file()
>  - t6030: explicitly test for bisection cleanup
>  - bisect--helper: `bisect_clean_state` shell function in C
>  - bisect--helper: `write_terms` shell function in C
>  - bisect: rewrite `check_term_format` shell function in C
>  - bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
>
>  GSoC "bisect" topic.

You could squash your commit. Thanks!

Regards,
Pranit Bauva

Re: [PATCH v4 08/25] sequencer: completely revamp the "todo" script parsing

[Junio C Hamano]

Johannes Schindelin  writes:

> - for (i = 1; *p; i++) {
> + for (i = 1; *p; i++, p = next_p) {
>   char *eol = strchrnul(p, '\n');
> - commit = parse_insn_line(p, eol, opts);
> - if (!commit)
> - return error(_("Could not parse line %d."), i);
> - next = commit_list_append(commit, next);
> - p = *eol ? eol + 1 : eol;
> +
> + next_p = *eol ? eol + 1 /* strip LF */ : eol;

This one was explained as "skip LF" in the previous round, and that
is more correct than "strip", I think.  The +1 here is not done to
"strip" the LF out of the end result, but to "skip" one to move to
the beginning of the next line.

The one in v3 08/25 decremented the pointer to denote the end of the
line with the explicit purpose of not including the CR in the end
result, which was explained as "skip CR", but it was stripping CR.
Correcting that explanation to "strip" was a right fix and I think
your v4 09/25 still has it, which is good.

Other than this unnecessary change since the previous round, I
didn't spot a difference in this step, which was already good.

Thanks.

[RFD] should all merge bases be equal?

[Junio C Hamano]

People can see how fast the usual merges I see everyday are by
looking at output from

$ git log --first-parent --format='%ct %s' master..pu

and noticing the seconds since epoch.  Most of the days, these are
recreated directly on top of 'master' from scratch, and they get
timestamps that are very close to each other (or the same), meaning
that I am getting multiple merges per second.

Being accustomed how fast my merges go, there is one merge that
hiccups for me every once in a few days: merging back from 'master'
to 'next'.  This happens after having multiple topics (that by
definition have to be the ones that were already merged to 'next'
some time ago) to 'master', and 'master' may also have its own
commit (e.g. update to "RelNotes") and merges of side branches that
were not in 'next' (e.g. merge from submaintainers like i18n, etc.)

The reason why this merge is slow is because it typically have many
merge bases.  For example, today's tip of 'next' is:

commit 6021889cc14df07d4366829367d2c4a11d1eaa56
Merge: 4868def05e 659889482a
Author: Junio C Hamano 
Date:   Mon Oct 17 14:02:05 2016 -0700

Sync with master

* master:
  Tenth batch for 2.11
  l10n: de.po: translate 260 new messages
  l10n: de.po: fix translation of autostash
  l10n: ru.po: update Russian translation

which is a merge that has 12 merge bases:

$ git merge-base --all 4868def05e 659889482a | git name-rev --stdin
3cdd5d19178a54d2e51b5098d43b57571241d0ab (ko/master)
641c900b2c3a8c3d385eb353b3801a5f49ddbb47 (js/reset-usage)
30cfe72d37ed8c174cae43923769730a94549dae (rs/pretty-format-color-doc-fix)
654311bf6ee0fbf530c088271c2c76d46f31f82d (da/mergetool-diff-order)
72710165c932edb2b8552aef7aef2f357dde4746 (sb/submodule-config-doc-drop-path)
842a516cb02a53cf0291ff67ed6f8517966345c0 (js/regexec-buf)
62fe0eb4804c297486a1d421a4f893865fcbc911 (jk/quarantine-received-objects)
a94bb683970a111b467a36590ca36e52754ad504 (rs/cocci)
e8c42cb9ce6a566aad797cc6c5bc1279d608d819 (jk/ref-symlink-loop)
22d3b8de1b625813faec6f3d6ffe66124837b78b (jk/clone-copy-alternates-fix)
7431596ab1f05a13adb93b44108f27cfd6578a31 (nd/commit-p-doc)
5275c3081c2b2c6166a2fc6b253a3acb20f8ae89 (dt/http-empty-auth)

The tip of each topic that was merged recently to 'master' since
'master' was last merged to 'next' becomes a valid merge-base by
design of the workflow.  We merge a topic to 'master' whose tip
has been already in 'next' for a while, so the tip of 'next' before
merging 'master' back is a descendant of the tips of these topics,
and the tip of 'master' before I make this merge has just become a
descendant of the tips of these topics.  That makes them common
ancestors between 'master' and 'next'.

But for the purpose of figuring out the 3-way merge result, I
suspect that they are pretty much useless common ancestor to use as
the merge base.  The first one in the list, the old 'master' that
was merged the last time to 'next', would be a lot more useful one.

And of course, if I do this:

$ git checkout next
$ git merge master ;# this is slow due to the 12-base above
$ git checkout HEAD^ ;# detach at the previous state
$ git merge-recursive ko/master -- HEAD master

the merge is instantaneous.  I'd get only what truly happened
uniquely on 'master', e.g. RelNotes update and i18n merge.

I am wondering if it is worth trying to optimize it by taking
advantage of the specific workflow (i.e. give me an option to use
when merging 'master' back to 'next') and allows me to exclude the
tips of these topic branches.  Would I be making the result open to
the criss-cross merge gotchas the "recursive merge" machinery was
designed to prevent in the first place by doing so?  Offhand, I do
not think that would be the case.

Assuming that it is a good idea, there is another question of how to
tell the more meaningful merge bases (ko/master in this case) out of
the less useful ones (all the others).  I think it would be
sufficient to reject commits that are not on the first-parent chain
of neither branch being merged.  Among the 12 merge bases, ko/master
is the only one that appears on the first-parent chain from 'master'
being merged (it does not appear on the first-parent chain from
'next').  All others were topic tips that by definition were merged
as second parent to integration branches ('next' and later 'master').
The place to do this would be a new option to 'merge-base'; instead
of "--all", perhaps "--major" option gives only the major merge bases
(with the definition of 'major' being the above heuristics), and then
"git merge-recursive" would learn "-Xmajor-base-only" strategy option,
or something along that line.

Thoughts?

What's cooking in git.git (Oct 2016, #04; Mon, 17)

[Junio C Hamano]

Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.  The ones marked with '.' do not appear in any of
the integration branches, but I am still holding onto them.

We may have to start thinking about 2.10.2; there are about a dozen
and half fixes accumulated on the 'maint' front.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[Graduated to "master"]

* da/mergetool-diff-order (2016-10-11) 4 commits
  (merged to 'next' on 2016-10-11 at 3d1b98c16d)
 + mergetool: honor -O
 + mergetool: honor diff.orderFile
 + mergetool: move main program flow into a main() function
 + mergetool: add copyright

 "git mergetool" learned to honor "-O" to control the
 order of paths to present to the end user.


* dt/http-empty-auth (2016-10-04) 1 commit
  (merged to 'next' on 2016-10-10 at 10b7b0a6a5)
 + http: http.emptyauth should allow empty (not just NULL) usernames

 http.emptyauth configuration is a way to allow an empty username to
 pass when attempting to authenticate using mechanisms like
 Kerberos.  We took an unspecified (NULL) username and sent ":"
 (i.e. no username, no password) to CURLOPT_USERPWD, but did not do
 the same when the username is explicitly set to an empty string.


* jk/alt-odb-cleanup (2016-10-10) 18 commits
  (merged to 'next' on 2016-10-10 at d2ed6b6d21)
 + alternates: use fspathcmp to detect duplicates
 + sha1_file: always allow relative paths to alternates
 + count-objects: report alternates via verbose mode
 + fill_sha1_file: write into a strbuf
 + alternates: store scratch buffer as strbuf
 + fill_sha1_file: write "boring" characters
 + alternates: use a separate scratch space
 + alternates: encapsulate alt->base munging
 + alternates: provide helper for allocating alternate
 + alternates: provide helper for adding to alternates list
 + link_alt_odb_entry: refactor string handling
 + link_alt_odb_entry: handle normalize_path errors
 + t5613: clarify "too deep" recursion tests
 + t5613: do not chdir in main process
 + t5613: whitespace/style cleanups
 + t5613: use test_must_fail
 + t5613: drop test_valid_repo function
 + t5613: drop reachable_via function
 (this branch is used by jk/quarantine-received-objects.)

 Codepaths involved in interacting alternate object store have
 been cleaned up.


* jk/clone-copy-alternates-fix (2016-10-05) 1 commit
  (merged to 'next' on 2016-10-10 at 8154134c8c)
 + clone: detect errors in normalize_path_copy

 "git clone" of a local repository can be done at the filesystem
 level, but the codepath did not check errors while copying and
 adjusting the file that lists alternate object stores.


* jk/quarantine-received-objects (2016-10-10) 5 commits
  (merged to 'next' on 2016-10-10 at 0fd3e3b2ef)
 + tmp-objdir: do not migrate files starting with '.'
 + tmp-objdir: put quarantine information in the environment
 + receive-pack: quarantine objects until pre-receive accepts
 + tmp-objdir: introduce API for temporary object directories
 + check_connected: accept an env argument
 (this branch uses jk/alt-odb-cleanup.)

 In order for the receiving end of "git push" to inspect the
 received history and decide to reject the push, the objects sent
 from the sending end need to be made available to the hook and
 the mechanism for the connectivity check, and this was done
 traditionally by storing the objects in the receiving repository
 and letting "git gc" to expire it.  Instead, store the newly
 received objects in a temporary area, and make them available by
 reusing the alternate object store mechanism to them only while we
 decide if we accept the check, and once we decide, either migrate
 them to the repository or purge them immediately.


* jk/ref-symlink-loop (2016-10-10) 2 commits
  (merged to 'next' on 2016-10-11 at ac5c35f87f)
 + files_read_raw_ref: prevent infinite retry loops in general
 + files_read_raw_ref: avoid infinite loop on broken symlinks

 A stray symbolic link in $GIT_DIR/refs/ directory could make name
 resolution loop forever, which has been corrected.


* js/regexec-buf (2016-10-10) 1 commit
  (merged to 'next' on 2016-10-11 at 466a26548c)
 + configure.ac: improve description of NO_REGEX test

 A follow-up to an already graduated topic.


* js/reset-usage (2016-10-11) 1 commit
  (merged to 'next' on 2016-10-11 at 61ad4a7c0e)
 + reset: fix usage

 Message fix-up.


* nd/commit-p-doc (2016-10-05) 1 commit
  (merged to 'next' on 2016-10-10 at 5a9996dd7b)
 + git-commit.txt: clarify --patch mode with pathspec

 Documentation for "git commit" was updated to clarify that "commit
 -p " adds to the current contents of the index to come up
 with what to commit.


* rs/cocci (2016-10-10) 2 commits
  (merged to 'next' on 2016-10-11 at bbd6a88402)
 + use strbuf_add_unique_abbrev() for adding 

[PATCH] submodule--helper: normalize funny urls

[Stefan Beller]

Currently a URL for the superproject ending in

(A).../path/to/dir
(B).../path/to/dir/
(C).../path/to/dir/.
(D).../path/to/dir/./.
(E).../path/to/dir/.///.//.

is treated the same in (A) and (B), but (C, D, E) are different.

We never produce the URLs in (C,D,E) ourselves, they come to use, because
the user used it as the URL for cloning a superproject.
Normalize these paths.

Signed-off-by: Stefan Beller 
---

 By being strict in Git, I think we also fix the Git for Windows painpoints.
 
 This goes on top of origin/sb/submodule-ignore-trailing-slash.
 
 Thanks,
 Stefan

 builtin/submodule--helper.c | 49 ++---
 t/t0060-path-utils.sh   | 11 ++
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 260f46f..ca90763 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,30 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
 }
 
+static void strip_url_ending(char *url, size_t *_len)
+{
+   int check_url_stripping = 1;
+   size_t len = _len ? *_len : strlen(url);
+
+   while (check_url_stripping) {
+   check_url_stripping = 0;
+   if (is_dir_sep(url[len-2]) && url[len-1] == '.') {
+   url[len-2] = '\0';
+   len -= 2;
+   check_url_stripping = 1;
+   }
+
+   if (is_dir_sep(url[len-1])) {
+   url[len-1] = '\0';
+   len --;
+   check_url_stripping = 1;
+   }
+   }
+
+   if (_len)
+   *_len = len;
+}
+
 /*
  * The `url` argument is the URL that navigates to the submodule origin
  * repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +117,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
  * the superproject working tree otherwise.
  *
  * NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url  url  outcome  expectation
- * http://a.com/b  ../c http://a.com/c   as is
- * http://a.com/b/ ../c http://a.com/c   same as previous line, but
- *   ignore trailing slash in 
url
- * http://a.com/b  ../../c  http://c error out
- * http://a.com/b  ../../../c   http:/c  error out
- * http://a.com/b  ../../../../chttp:c   error out
- * http://a.com/b  ../../../../../c.:c   error out
+ * remote_url   url  outcome  expectation
+ * http://a.com/b   ../c http://a.com/c   as is
+ * http://a.com/b/  ../c http://a.com/c   same as previous line, 
but
+ *ignore trailing '/' in 
url
+ * http://a.com/b/. ../c http://a.com/c   same as previous line, 
but
+ *ignore trailing '/.' in 
url
+ * http://a.com/b   ../../c  http://c error out
+ * http://a.com/b   ../../../c   http:/c  error out
+ * http://a.com/b   ../../../../chttp:c   error out
+ * http://a.com/b   ../../../../../c.:c   error out
  * NEEDSWORK: Given how chop_last_dir() works, this function is broken
  * when a local part has a colon in its path component, too.
  */
@@ -115,8 +141,7 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
 
-   if (is_dir_sep(remoteurl[len-1]))
-   remoteurl[len-1] = '\0';
+   strip_url_ending(remoteurl, );
 
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
@@ -149,10 +174,10 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset();
strbuf_addf(, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
-   if (ends_with(url, "/"))
-   strbuf_setlen(, sb.len - 1);
free(remoteurl);
 
+   strip_url_ending(sb.buf, );
+
if (starts_with_dot_slash(sb.buf))
out = xstrdup(sb.buf + 2);
else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" 
"../submodule" "submodule"
 test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" 
"//somewhere else/subrepo"
 test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" 
"../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
 test_submodule_relative_url "(null)" "$PWD/super_update_r2" 
"../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" 

Re: [PATCH] t0040: convert all possible tests to use `test-parse-options --expect`

[Junio C Hamano]

Pranit Bauva  writes:

> Use "test-parse-options --expect" to rewrite the tests to avoid checking
> the whole variable dump by just testing what is required. This commit is
> based on 8ca65aeb (t0040: convert a few tests to use test-parse-options;
> Junio C Hamano; May 6, 2016).
>
> Signed-off-by: Pranit Bauva 
> ---
>  t/t0040-parse-options.sh | 183 
> ---
>  1 file changed, 13 insertions(+), 170 deletions(-)

Whoa.  Quite a lot of repetitions removed.

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

[Ramsay Jones]

On 17/10/16 21:07, Junio C Hamano wrote:
> Ramsay Jones  writes:
> 
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>>  find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
> 
> "git clean -x" anybody?

I don't use 'git clean' because, on the very few occasions that I have
tried to use it, it always deletes _far_ more than I thought it would
or should. (particularly config.mak). Hmm, "git clean -X -- '*.o'"
_might_ do what I want, but 'find' is so much easier ... :-D

ATB,
Ramsay Jones

Problems with "git svn clone"

[K Richard Pixley]

I'm trying to create a git-svn repository with:

git svn clone --username=pixleyr --stdlayout --branches branches --branches 
branches2 svn://mumble.com/mumble/mumble

but the command dies about 11seconds in with:

A   
src/kernel/ppc/2.4.26-pre5-moto-pq3-2004_06_04-0/drivers/usb/serial/belkin_sa.h

A   
src/kernel/ppc/2.4.26-pre5-moto-pq3-2004_06_04-0/drivers/usb/serial/keyspan_usa28_fw.h

error: git-svn died of signal 11

This seems awfully early and blatant for a core dump.  What can I do to
get this running?

Initially discovered on git-2.7.4, (ubuntu-16.04), but also reproduced
on freshly built top of tree git-2.10.1.445.g3cdd5d1.

--rich




- CONFIDENTIAL-

This email and any files transmitted with it are confidential, and may also be 
legally privileged. If you are not the intended recipient, you may not review, 
use, copy, or distribute this message. If you receive this email in error, 
please notify the sender immediately by reply email and then delete this email.

Re: [PATCH] cocci: avoid self-references in object_id transformations

[Junio C Hamano]

René Scharfe  writes:

> I get these instead with 6513eabcbcbcfa684d4bb2d57f61c662b870b5ca on
> Debian testing with its "spatch version 1.0.4 with Python support and
> with PCRE support", which look legit:

They do look good.  So I'd stop worrying about it and see how
painful to update my copy of spatch would be.

Thanks.

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

[Stefan Beller]

On Mon, Oct 17, 2016 at 1:07 PM, Junio C Hamano  wrote:
> Ramsay Jones  writes:
>
>> Heh, I actually have the following in my config.mak already:
>>
>> extra-clean: clean
>>   find . -iname '*.o' -exec rm {} \;
>>
>> But for some reason I _always_ type 'make clean' and then, to top
>> it off, I _always_ type the 'find' command by hand (I have no idea
>> why) :-D
>
> "git clean -x" anybody?

I only want to cleanup compiled stuff and such, not the precious
unversioned text files I have laying around here. So I guess

  git clean -x -e:(attr:!binary)

would suffice, though. ;)

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

[René Scharfe]

Am 17.10.2016 um 22:07 schrieb Junio C Hamano:

Ramsay Jones  writes:


Heh, I actually have the following in my config.mak already:

extra-clean: clean
find . -iname '*.o' -exec rm {} \;

But for some reason I _always_ type 'make clean' and then, to top
it off, I _always_ type the 'find' command by hand (I have no idea
why) :-D


"git clean -x" anybody?


This removes config.mak as well.

René

Re: [GIT PULL] l10n updates for 2.10.0 maint branch

[Junio C Hamano]

Jiang Xin  writes:

> Hi Junio,
>
> Please pull the following l10n updates for Git 2.10 to the maint branch.
>
> The following changes since commit 9a4b694c539fead26833c2104c1a93d3a2b4c50a:
>
>   l10n: zh_CN: review for git v2.10.0 l10n (2016-09-11 21:34:23 +0800)

Thanks.

> Dimitriy Ryazantcev (1):
>   l10n: ru.po: update Russian translation
>
> Jiang Xin (1):
>   Merge branch 'russian-l10n' of https://github.com/DJm00n/git-po-ru
>
> Ralf Thielow (2):
>   l10n: de.po: fix translation of autostash
>   l10n: de.po: translate 260 new messages
>
>  po/de.po | 5182 
> +-
>  po/ru.po |   52 +-
>  2 files changed, 2815 insertions(+), 2419 deletions(-)
>
> --
> Jiang Xin

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Junio C Hamano]

Johannes Sixt  writes:

>> "../." taken relative to "$(pwd)/." must be "$(pwd)/."
>> "../." taken relative to "$PWD/." must be "$(pwd)/."
>>
>> test, because of the limitation of your bash, cannot have the latter
>> half of the pair, so you'd need to comment it out with in-code
>> explanation, perhaps?
>
> No, we do not have to test both cases. Git, the program, never sees
> Unixy input. It cannot distinguish the two cases.

Thanks.

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

[Junio C Hamano]

Ramsay Jones  writes:

> Heh, I actually have the following in my config.mak already:
>
> extra-clean: clean
>   find . -iname '*.o' -exec rm {} \;
>
> But for some reason I _always_ type 'make clean' and then, to top
> it off, I _always_ type the 'find' command by hand (I have no idea
> why) :-D

"git clean -x" anybody?

Re: [PATCH] cocci: avoid self-references in object_id transformations

[René Scharfe]

Am 17.10.2016 um 20:08 schrieb Junio C Hamano:
> ... oops.  Totally unrelated to this patch, but I see these in
> strbuf.cocci.patch (this is at the tip of 'pu'), which are total
> nonsense.  Perhaps I am running a way-stale spatch?  It claims to be
> "spatch version 1.0.0-rc19 with Python support and with PCRE support"
> 
> --- date.c
> +++ /tmp/cocci-output-21568-bd3448-date.c
> @@ -179,7 +179,7 @@ const char *show_date(unsigned long time
>  
>   if (mode->type == DATE_UNIX) {
>   strbuf_reset();
> - strbuf_addf(, "%lu", time);
> + strbuf_addstr(, time);
>   return timebuf.buf;
>   }
>  
> --- log-tree.c
> +++ /tmp/cocci-output-21608-b02087-log-tree.c
> @@ -400,7 +400,7 @@ void log_write_email_headers(struct rev_
>   extra_headers = subject_buffer;
>  
>   if (opt->numbered_files)
> - strbuf_addf(, "%d", opt->nr);
> + strbuf_addstr(, opt->nr);
>   else
>   fmt_output_commit(, commit, opt);
>   snprintf(buffer, sizeof(buffer) - 1,

I get these instead with 6513eabcbcbcfa684d4bb2d57f61c662b870b5ca on
Debian testing with its "spatch version 1.0.4 with Python support and
with PCRE support", which look legit:

--- sequencer.c
+++ /tmp/cocci-output-40365-db7a71-sequencer.c
@@ -159,7 +159,7 @@ int sequencer_remove_state(struct replay
free(opts->xopts[i]);
free(opts->xopts);
 
-   strbuf_addf(, "%s", get_dir(opts));
+   strbuf_addstr(, get_dir(opts));
remove_dir_recursively(, 0);
strbuf_release();
 
--- builtin/branch.c
+++ /tmp/cocci-output-40858-a86d1a-branch.c
@@ -316,7 +316,7 @@ static char *build_format(struct ref_fil
 
if (filter->verbose) {
strbuf_addf(, 
"%%(align:%d,left)%%(refname:strip=2)%%(end)", maxwidth);
-   strbuf_addf(, "%s", branch_get_color(BRANCH_COLOR_RESET));
+   strbuf_addstr(, branch_get_color(BRANCH_COLOR_RESET));
strbuf_addf(, " %%(objectname:short=7) ");
 
if (filter->verbose > 1)

Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+

[Ævar Arnfjörð Bjarmason]

On Mon, Oct 17, 2016 at 6:54 PM, Junio C Hamano  wrote:
> Ævar Arnfjörð Bjarmason  writes:
>
>> As far as I can tell the only outstanding "change this" is your
>> s/SHA1/SHA-1/ in , do
>> you want to fix that up or should I submit another series?
>
> I think I did that already myself while queuing.  Could you fetch
> what I queued on 'pu' to double check?

Thanks, looked at it, looks good to me!
> I think the diff between what was posted and what is queued (I just
> checked) looks like this:
>
> -gitweb: Link to 7-char+ SHA1s, not only 8-char+
> +gitweb: link to 7-char+ SHA-1s, not only 8-char+
>
>  Change the minimum length of an abbreviated object identifier in the
>  commit message gitweb tries to turn into link from 8 hexchars to 7.
>
> @@ -5,16 +12,18 @@
>  SHA-1 in commit log message links to "object" view", 2006-12-10), but
>  the default abbreviation length is 7, and has been for a long time.
>
> -It's still possible to reference SHA1s down to 4 characters in length,
> +It's still possible to reference SHA-1s down to 4 characters in length,
>  see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make
>  git actually produce that, so I doubt anyone is putting that into log
> -messages in practice, but people definitely do put 7 character SHA1s
> +messages in practice, but people definitely do put 7 character SHA-1s
>  into log messages.
>
>  I think it's fairly dubious to link to things matching [0-9a-fA-F]
>  here as opposed to just [0-9a-f], that dates back to the initial
>  version of gitweb from 161332a ("first working version",
> -2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
> +2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce
>  them as far as I can tell.
>
>  Signed-off-by: Ævar Arnfjörð Bjarmason 
> +Acked-by: Jakub Narębski 
> +Signed-off-by: Junio C Hamano 

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Johannes Sixt]

Am 17.10.2016 um 09:10 schrieb Junio C Hamano:

And I agree from that point of view that having to spell both sides
as $(pwd) would mean you are not testing that "Unixy input to
Windowsy output" expectation, but at the same time, I think you
would want "Windowsy input to Windowsy output" combination also does
produce correct result, which is not tested in the three tests shown
above.  IOW, probably you would want to test both (at least on any
platform where $PWD and $(pwd) textually disagree) for all these
[*1*], and the pair

"../." taken relative to "$(pwd)/." must be "$(pwd)/."
"../." taken relative to "$PWD/." must be "$(pwd)/."

test, because of the limitation of your bash, cannot have the latter
half of the pair, so you'd need to comment it out with in-code
explanation, perhaps?


No, we do not have to test both cases. Git, the program, never sees 
Unixy input. It cannot distinguish the two cases. If we did both tests, 
we would just test that *if* the front-end of git is an MSYS program 
(such as bash), *then* that MSYS front-end has converted the Unixy path 
to a Windows-y path in such a way that the end-result is also as 
expected. It's similar to a test that grep produces expected output.


I think that we could reduce the confusion by converting all $PWD to 
$(pwd) in these test cases. I don't remember why I suggested to use $PWD 
for one of the arguments of the test cases (the second must be $(pwd)), 
but the most likely reason is only that we save a process.


-- Hannes

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Junio C Hamano]

Stefan Beller  writes:

>> Where at the end-user facing level does this trailing "/." surface
>> and how does the difference appear to them?  I think that is the
>> crucial question.
>>
>> Unless there is some convincing argument why "." is not special
>> (i.e. counter-argument to the above "bus vs sub" and ". vs sub"
>> example), I would think "existing users with /." does not matter.
>> If they are "relying" on the behaviour, I would think it is not
>> because they find that behaviour intuitive, but only because they
>> learned to live with it.  IOW, treating all of A/B/C the same way
>> would appear to them a strict bugfix, I would think.
>
> I see, so we should adapt the windows style and chop off '/.'
> to make A,B,C all the same, because internally we never produced
> C AFAICT.
>
> These came in via hand edited .gitmodules files.

Can you elaborate a bit more on this?  

Without seeing "The user added X/. instead of the usual X because
s/he wanted to see Y happen.  If s/he had X there, Z would have
happened instead of Y" and why being able to expect Y to happen is a
good thing (compared to Z), we may fail to notice why the more
"intuitive" (at least to me) "these three must result in the same
outcome: path/to/dir, path/to/dir/, or path/to/dir/." does not serve
a legitimate use case.

Re: [PATCH v4 00/25] Prepare the sequencer for the upcoming rebase -i patches

[Junio C Hamano]

Johannes Schindelin  writes:

> This patch series marks the '4' in the countdown to speed up rebase -i
> by implementing large parts in C (read: there will be three more patch
> series after that before the full benefit hits git.git: sequencer-i,
> rebase--helper and rebase-i-extra). It is based on the `libify-sequencer`
> patch series I submitted earlier.

The difference between the end results of v3 and v4 looked OK
(except the 08/25 "strip LF" change that is unneeded) to me, so I'll
skip the early part of the series from my review that correspond to
the ones in v3 that I've already reviewed and found good, and
continue from the later ones.

Thanks.

Re: [PATCH v4 05/25] sequencer: eventually release memory allocated for the option values

[Junio C Hamano]

Johannes Schindelin  writes:

> The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
> like a one-shot command when it reads its configuration: memory is
> allocated and released only when the command exits.
>
> This is kind of okay for git-cherry-pick, which *is* a one-shot
> command. All the work to make the sequencer its work horse was
> done to allow using the functionality as a library function, though,
> including proper clean-up after use.
>
> To remedy that, we now take custody of the option values in question,
> requiring those values to be malloc()ed or strdup()ed

That is the approach this patch takes, so "eventually release" in
the title is no longer accurate, I would think.

> Sadly, the current approach makes the code uglier, as we now have to
> take care to strdup() the values passed via the command-line.

I obviously disagree with that statement and the _entrust was too
ugly to live, but it is obviously subjective, and it boils down to
who has a better taste.  Let's not go there.

> +
> + /* These option values will be free()d */
> + opts->gpg_sign = xstrdup_or_null(opts->gpg_sign);
> + opts->strategy = xstrdup_or_null(opts->strategy);

xstrdup-or-null does make things cleaner.

> +static int git_config_string_dup(char **dest,
> +  const char *var, const char *value)
> +{
> + if (!value)
> + return config_error_nonbool(var);
> + free(*dest);
> + *dest = xstrdup(value);
> + return 0;
> +}

So does this.

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Stefan Beller]

On Mon, Oct 17, 2016 at 11:28 AM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> In any case, I find it more disturbing that we somehow ended up with
>>> a system where these three things are expected to behave differently:
>>>
>>> A - path/to/dir
>>> B - path/to/dir/
>>> C - path/to/dir/.
>>>
>>> Is that something we can fix?
>>
>> Well A, B are the same.
>> C is "obviously" different, when it comes to counting slashes for relative
>> path/url purposes, in the way that there are characters after the last slash
>> and just by coincidence '.' refers to the directory itself, C behaving like
>> 'path/to/dir/sub' seems right to me.
>
> It doesn't look right to me at all.  If you were contrasting
>
> cd path/to/dir/sub && cd ..
> cd path/to/dir/bus && cd ..
>
> then I would understand, but why should these two
>
> cd path/to/dir/. && cd ..
> cd path/to/dir/sub && cd ..
>
> behave the same?
>
>> So how do you imagine this fix going forward?
>> * Breaking existing users with /. at the end? by treating it the same as A,B
>> * Do some check based on time/version of Git and cover the old data?
>> * Forbid /. at the end from now on?
>
> Where at the end-user facing level does this trailing "/." surface
> and how does the difference appear to them?  I think that is the
> crucial question.
>
> Unless there is some convincing argument why "." is not special
> (i.e. counter-argument to the above "bus vs sub" and ". vs sub"
> example), I would think "existing users with /." does not matter.
> If they are "relying" on the behaviour, I would think it is not
> because they find that behaviour intuitive, but only because they
> learned to live with it.  IOW, treating all of A/B/C the same way
> would appear to them a strict bugfix, I would think.

I see, so we should adapt the windows style and chop off '/.'
to make A,B,C all the same, because internally we never produced
C AFAICT.

These came in via hand edited .gitmodules files.

>
> It is totally a different matter if OUR code that consumes the
> output from the submodule-helper --resolve-relative" internally is
> confused and relies on "../. relative to path/to/dir/. is the same
> as ../. relative to path/to/dir/sub" for whatever reason.  Without
> fixing that, I would not surprised if fixing things to treat A/B/C
> the same way would surface differences in the end-user observable
> behaviour in a negative way.
>

Re: [PATCH v11 00/14] Git filter protocol

[Junio C Hamano]

larsxschnei...@gmail.com writes:

> The goal of this series is to avoid launching a new clean/smudge filter
> process for each file that is filtered.
>
> A short summary about v1 to v5 can be found here:
> https://git.github.io/rev_news/2016/08/17/edition-18/
>
> This series is also published on web:
> https://github.com/larsxschneider/git/pull/15
>
> Patches 1 and 2 are cleanups and not strictly necessary for the series.
> Patches 3 to 12 are required preparation. Patch 13 is the main patch.
> Patch 14 adds an example how to use the Git filter protocol in contrib.

Will replace.  If you ever need tor reroll 13, please squash the
following in (which I already did locally so there is no need to
resend only to correct it).

Thanks.

 convert.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/convert.c b/convert.c
index 9d2aa68df9..bc242276ff 100644
--- a/convert.c
+++ b/convert.c
@@ -535,7 +535,8 @@ static int packet_write_list(int fd, const char *line, ...)
return packet_flush_gently(fd);
 }
 
-static void read_multi_file_filter_status(int fd, struct strbuf *status) {
+static void read_multi_file_filter_status(int fd, struct strbuf *status)
+{
struct strbuf **pair;
char *line;
for (;;) {
-- 
2.10.1-613-g6ad57fc60c

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Junio C Hamano]

Stefan Beller  writes:

>> In any case, I find it more disturbing that we somehow ended up with
>> a system where these three things are expected to behave differently:
>>
>> A - path/to/dir
>> B - path/to/dir/
>> C - path/to/dir/.
>>
>> Is that something we can fix?
>
> Well A, B are the same.
> C is "obviously" different, when it comes to counting slashes for relative
> path/url purposes, in the way that there are characters after the last slash
> and just by coincidence '.' refers to the directory itself, C behaving like
> 'path/to/dir/sub' seems right to me.

It doesn't look right to me at all.  If you were contrasting

cd path/to/dir/sub && cd ..
cd path/to/dir/bus && cd ..

then I would understand, but why should these two

cd path/to/dir/. && cd ..
cd path/to/dir/sub && cd ..

behave the same?

> So how do you imagine this fix going forward?
> * Breaking existing users with /. at the end? by treating it the same as A,B
> * Do some check based on time/version of Git and cover the old data?
> * Forbid /. at the end from now on?

Where at the end-user facing level does this trailing "/." surface
and how does the difference appear to them?  I think that is the
crucial question.

Unless there is some convincing argument why "." is not special
(i.e. counter-argument to the above "bus vs sub" and ". vs sub"
example), I would think "existing users with /." does not matter.
If they are "relying" on the behaviour, I would think it is not
because they find that behaviour intuitive, but only because they
learned to live with it.  IOW, treating all of A/B/C the same way
would appear to them a strict bugfix, I would think.

It is totally a different matter if OUR code that consumes the
output from the submodule-helper --resolve-relative" internally is
confused and relies on "../. relative to path/to/dir/. is the same
as ../. relative to path/to/dir/sub" for whatever reason.  Without
fixing that, I would not surprised if fixing things to treat A/B/C
the same way would surface differences in the end-user observable
behaviour in a negative way.

Re: [PATCH] cocci: avoid self-references in object_id transformations

[Junio C Hamano]

"brian m. carlson"  writes:

> On Sat, Oct 15, 2016 at 10:25:34AM +0200, René Scharfe wrote:
>> The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
>> oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
>> sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively.  Make sure
>> that the Coccinelle transformations for converting legacy function calls
>> are not applied to these wrappers themselves, which would result in
>> tautological declarations.
>
> Ah, yes, this is a good idea.  I've had to hack around this, but this is
> much better than having to fix it up by hand.

Yes, seeing an empty *.cocci.patch files after running coccicheck is
a great feeling, but without something like this patch, we can never
reach that goal ;-)

Thanks.

... oops.  Totally unrelated to this patch, but I see these in
strbuf.cocci.patch (this is at the tip of 'pu'), which are total
nonsense.  Perhaps I am running a way-stale spatch?  It claims to be
"spatch version 1.0.0-rc19 with Python support and with PCRE support"

--- date.c
+++ /tmp/cocci-output-21568-bd3448-date.c
@@ -179,7 +179,7 @@ const char *show_date(unsigned long time
 
if (mode->type == DATE_UNIX) {
strbuf_reset();
-   strbuf_addf(, "%lu", time);
+   strbuf_addstr(, time);
return timebuf.buf;
}
 
--- log-tree.c
+++ /tmp/cocci-output-21608-b02087-log-tree.c
@@ -400,7 +400,7 @@ void log_write_email_headers(struct rev_
extra_headers = subject_buffer;
 
if (opt->numbered_files)
-   strbuf_addf(, "%d", opt->nr);
+   strbuf_addstr(, opt->nr);
else
fmt_output_commit(, commit, opt);
snprintf(buffer, sizeof(buffer) - 1,

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Stefan Beller]

On Mon, Oct 17, 2016 at 12:10 AM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>>> > expecting success:
>>> > actual=$(git submodule--helper resolve-relative-url-test 
>>> > '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') 
>>> > &&
>>> > test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash 
>>> > directory.t0060-path-utils/.'
>>> >
>>> > +++ git submodule--helper resolve-relative-url-test '(null)' 
>>> > '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
>>> > ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
>>> > ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 
>>> > 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'
>
> This may well be total misunderstanding on my side, but is the
> expectation of this test even correct?  If it wants to take "../."
> relative to "$LEAD/t/trash utils/.", should't it go one level up
> with ".." to $LEAD/t and then stay there with ".", expecting
> "$LEAD/t" which is what the above is giving us?
>
> IOW, the above makes me wonder why having one of these as the base
>
> A - path/to/dir
> B - path/to/dir/
> C - path/to/dir/.
>
> to resolve the relative "../." give different results.

Because the shell script originally did "just"

relative="../."
if path/to/dir/ ends with slash, chop it off.
while $relative starts with "../";
do
chop off starting '../' of relative
chop of last '/' and following from "path/to/dir/."
done

(Linux:)
As B was made to A first, only C differs as a result, because
you had one more '/' in there.

(Windows:)
However Windows also detects '/.' (C) and makes it an A,
(in C only, because shell code was not treated Windows-sy)
which is where the incompatibility between Windows and other
platforms arises.

So we have a couple of choices (for Git)now:
* go back to using shell only for submodule things as that doesn't
  have the regression and it alos plays nicely with Git for Windows.
* use C for the submodule code in Git and revert the regression fix
  "because consistency across platforms trumps
  consistency over time"
* use C for the submodule code in Git and keep the regression fix
  "because consistency over time in Git proper is more important
  than playing nicely with Git for Windows"
* would it be possible to revert this to shell on Windows only?

> Whether bash
> on Windows removes the dot at the end of C to turn it into B, as
> long as A and B give us the same result we wouldn't be hitting the
> problem, no?

Well in Git proper A,B are the same and C is different.
(B was fixed as a regression)
In Windows C is like B, which was different without the regression
fix, but now it is the same as A, too.

>
>>> >  test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" 
>>> > "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
>>> >  test_submodule_relative_url "(null)" "$PWD/super_update_r2" 
>>> > "../subsuper_update_r" "$(pwd)/subsuper_update_r"
>>> > -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>>> > +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."
>
>>> > The reasons this is ugly: we specifically test for *Unixy* paths when we
>>> > use $PWD, as opposed to *Windowsy* paths when using $(pwd).
>
> Just to ensure I am following this correctly, two tests that come
> before the one you are touching above have $PWD on the input side
> and $(pwd) on the expectation side.  That is what you mean by the
> next paragraph, right?  They want to make sure that you honor the
> Unixy user input on Windows and still produce Windowsy result, that
> is.
>
>>> > We do this to
>>> > ensure a certain level of confidence that running things such as
>>> >
>>> > git clone --recurse-submodules /z/project/.
>>> >
>>> > work. And now that does not work anymore.
>
> And I agree from that point of view that having to spell both sides
> as $(pwd) would mean you are not testing that "Unixy input to
> Windowsy output" expectation, but at the same time, I think you
> would want "Windowsy input to Windowsy output" combination also does
> produce correct result, which is not tested in the three tests shown
> above.  IOW, probably you would want to test both (at least on any
> platform where $PWD and $(pwd) textually disagree) for all these
> [*1*], and the pair
>
> "../." taken relative to "$(pwd)/." must be "$(pwd)/."
> "../." taken relative to "$PWD/." must be "$(pwd)/."
>
> test, because of the limitation of your bash, cannot have the latter
> half of the pair, so you'd need to comment it out with in-code
> explanation, perhaps?  IOW something along the lines of...
>
>  -- >8 -- snip -- >8 --
>
> test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" 
> "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
> test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" 
> "../subsuper_update_r" "$(pwd)/subsuper_update_r"
> 

Re: Merge conflicts in .gitattributes can cause trouble

[Junio C Hamano]

Johannes Schindelin  writes:

> I would vote for:
>
> 4. We keep letting Git read in the *current* version of .gitattributes
>*before* the merge, and apply those attributes while performing the
>merge.

Even though this needs a major surgery to the way the attr subsystem
reads from these files, I think it is conceptually the cleanest.

Re: [RFC] Case insensitive Git attributes

[Junio C Hamano]

Duy Nguyen  writes:

> I agree. Which is why I wrote "we probably want something in the same
> spirit but limited to .gitattributes and .gitignore only". In other
> words we could have core.someName that makes .gitattributes and
> .gitignore patterns case-insensitive (or core-sensitive). If it's
> present, it overrides core.ignoreCase. If it's not present,
> core.ignoreCase decides. I'm just not sure if the new config should
> cover everything involving filename's case in git. That's too big to
> fit in my head.

Once I stopped thinking about this as "filename's case", it does fit
my head ;-)

I view the proposed knob as making patterns in .gitattributes and
.gitignore case insensitive, iow, it is a lazy and useful short-hand
for (mentally) editing "*.c attr" to "*.[cC] attr" without touching
these files.

And I agree that the knob that is missing in today's Git should
default to whatever core.ignoreCase's value is, iow, on case
insensitive filesystem, attr and ignore may match case insensitively
in today's Git, but when the knob is introduced, it should allow
forcing case sensitive match there by setting it to false, just like
the knob is proposed to be used in the oppositite direction to force
case insensitive match regardless of the case insensitiveness of the
underlying filesystem.

Re: [RFC] test-lib: detect common misuse of test_expect_failure

[Junio C Hamano]

Jeff King  writes:

> I like the general idea, but I'm not sure how this would interact with
> the tests in t that test the test suite.

I tried but gave up adding a new test for this to t ;-)

>>  test_expect_failure () {
>> +if test "$test_in_progress" = 1
>> +then
>> +error "bug in the test script: did you mean test_must_fail 
>> instead of test_expect_failure?"
>> +fi
>
> This follows existing practice for things like the &&-lint-checker, and
> bails out on the whole test script.

Yes, you guessed correctly where the above came from.

> That sometimes makes it hard to find
> the problematic test, especially if you're running via something like
> "prove", because it doesn't make valid TAP output.

Yeah, true.

> It might be nicer if we just said "this test is malformed, and therefore
> fails", and then you get all the usual niceties for recording and
> finding the failed test.
>
> I don't think it would be robust enough to try to propagate the error up
> to the outer test_expect_success block (and anyway, you'd also want to
> know about it in a test_expect_failure block; it's a bug in the test,
> not a known breakage). But perhaps error() could dump some TAP-like
> output with a "virtual" failed test.
>
> Something like:
> ...
> which lets "make prove" collect the broken test number.
>
> It would perhaps need to cover the case when $test_count is "0"
> separately. I dunno. It would be nicer still if we could continue
> running other tests in the script, but I think it's impossible to
> robustly jump back to the outer script.
>
> These kinds of "bug in the test suite" are presumably rare enough that
> the niceties don't matter that much, but I trigger the &&-checker
> reasonably frequently (that and test_line_count, because I can never
> remember the correct invocation).
>
> Anyway. That's all orthogonal to your patch. I just wondered if we could
> do better, but AFAICT the right way to do better is to hook into
> error(), which means your patch would not have to care exactly how it
> fails.

Yeah, the change to error() may be a good thing to do, but it has
quite a many callers in t/*lib*.sh and definitely deserves to be a
separate patch, not tied to this single test.

Re: [PATCH] fetch: use "quick" has_sha1_file for tag following

[Junio C Hamano]

Jeff King  writes:

> Still an impressive speedup as a percentage, but negligible in absolute
> terms. But that's on a local filesystem on a Linux machine. I'd worry
> much more about a system with a slow readdir(), e.g., due to NFS.
> Somebody's real-world NFS case[1] was what prompted us to do 0eeb077
> (index-pack: avoid excessive re-reading of pack directory, 2015-06-09).

Yes.

> It looks like I _did_ look into optimizing this into a single stat()
> call in the thread at [1]. I completely forgot about that. I did find
> there that naively using stat_validity() on a directory is racy, though
> I wonder if we could do something clever with gettimeofday() instead.

It feels funny to hear an idea to compare fs timestamp with gettimeofday
immedately after hearing the word NFS, though ;-).

>> I agree that the fallout from the inaccuracy of "quick" approach is
>> probably acceptable and the next "fetch" will correct it anyway, so
>> let's do the "quick but inaccurate" for now and perhaps cook it in
>> 'next' for a bit longer than other topics?
>
> I doubt that cooking in 'next' for longer will turn up anything useful.
> The case we care about is the race between a repack and a fetch. We
> lived with the "quick" version of has_sha1_file() everywhere for 8
> years.

A very convincing argument.  I stand corrected.

Thanks.

Re: [PATCH v3 16/25] sequencer: support amending commits

[Junio C Hamano]

Johannes Schindelin  writes:

> This teaches the run_git_commit() function to take an argument that will
> allow us to implement "todo" commands that need to amend the commit
> messages ("fixup", "squash" and "reword").

Likewise to 15/25, i.e. Good, though the growth by these two steps
starts to make me wonder if these three options should be crammed
into an unsigned "flags" bitword.

I see you have v4, so I'll ignore the remainder of this stale round
and start reading that updated one instead.

>
> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b621f4b..403a4f0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -481,7 +481,7 @@ static char **read_author_script(void)
>   * author metadata.
>   */
>  static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> -   int allow_empty, int edit)
> +   int allow_empty, int edit, int amend)
>  {
>   char **env = NULL;
>   struct argv_array array;
> @@ -510,6 +510,8 @@ static int run_git_commit(const char *defmsg, struct 
> replay_opts *opts,
>   argv_array_push(, "commit");
>   argv_array_push(, "-n");
>  
> + if (amend)
> + argv_array_push(, "--amend");
>   if (opts->gpg_sign)
>   argv_array_pushf(, "-S%s", opts->gpg_sign);
>   if (opts->signoff)
> @@ -785,7 +787,7 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   }
>   if (!opts->no_commit)
>   res = run_git_commit(opts->edit ? NULL : git_path_merge_msg(),
> -  opts, allow, opts->edit);
> +  opts, allow, opts->edit, 0);
>  
>  leave:
>   free_message(commit, );

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

[Ramsay Jones]

On 17/10/16 10:37, Jeff King wrote:
> On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:
> 
>>> Gross. I would not be opposed to a Makefile rule that outputs the
>>> correct set of OBJECTS so this (or other) scripts could build on it.
>>
>> You could also use the method I use in Git for Windows to "extend" the
>> Makefile:
>>
>> -- snipsnap --
>> cat >dummy.mak <> include Makefile
>>
>> blub: $(OBJECTS)
>>  do-something-with $^
>> EOF
>>
>> make -f dummy.mak blub
> 
> Hacky but clever. I like it.
> 
> In the particular case of git, I think I've cheated similarly before by
> putting things in config.mak, though of course an arbitrary script can't
> assume it can overwrite that file.

Heh, I actually have the following in my config.mak already:

extra-clean: clean
find . -iname '*.o' -exec rm {} \;

But for some reason I _always_ type 'make clean' and then, to top
it off, I _always_ type the 'find' command by hand (I have no idea
why) :-D

ATB,
Ramsay Jones

Re: [PATCH v3 14/25] sequencer: introduce a helper to read files written by scripts

[Junio C Hamano]

Johannes Schindelin  writes:

> +/*
> + * Reads a file that was presumably written by a shell script, i.e.
> + * with an end-of-line marker that needs to be stripped.
> + *
> + * Returns 1 if the file was read, 0 if it could not be read or does not 
> exist.
> + */
> +static int read_oneliner(struct strbuf *buf,
> + const char *path, int skip_if_empty)
> +...
> + if (strbuf_read_file(buf, path, 0) < 0) {
> + warning_errno(_("could not read '%s'"), path);
> + return 0;
> + }
> + if (buf->len > orig_len && buf->buf[buf->len - 1] == '\n') {
> + if (--buf->len > orig_len && buf->buf[buf->len - 1] == '\r')
> + --buf->len;
> + buf->buf[buf->len] = '\0';
> + }

The name says "oneliner" but this reads the whole thing and trims
only the last line of the input.  Which is correct?

Do we want to error out if we got more than one line?  That makes it
more strict.  Going in the other direction, do we want to just read
the first line and ignore the remainder?  That allows users to leave
cruft after what matters.  I _think_ the existing code is closer to
the latter, i.e. something along the lines of ...

struct strbuf oneline = STRBUF_INIT;
FILE *fp = fopen(path, "r");
if (!fp) {
warning_errno(_("could not open '%s'"), path);
return 0;
}
if (strbuf_getline(, fp) < 0)
; /* EOF - empty */
else {
strbuf_addbuf(buf, );
}

Re: [PATCH v3 15/25] sequencer: allow editing the commit message on a case-by-case basis

[Junio C Hamano]

Johannes Schindelin  writes:

> In the upcoming commits, we will implement more and more of rebase -i's
> functionality inside the sequencer. One particular feature of the
> commands to come is that some of them allow editing the commit message
> while others don't, i.e. we cannot define in the replay_opts whether the
> commit message should be edited or not.
>
> Let's add a new parameter to the run_git_commit() function. Previously,
> it was the duty of the caller to ensure that the opts->edit setting
> indicates whether to let the user edit the commit message or not,
> indicating that it is an "all or nothing" setting, i.e. that the
> sequencer wants to let the user edit *all* commit message, or none at
> all. In the upcoming rebase -i mode, it will depend on the particular
> command that is currently executed, though.

Makes tons of sense.

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

[Ramsay Jones]

On 17/10/16 03:18, Jeff King wrote:
> On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
> 
>> Hmm, well, you have to remember that 'make clean' sometimes
>> doesn't make clean. Ever since the Makefile was changed to only
>> remove $(OBJECTS), rather than *.o xdiff/*.o etc., you have to
>> remember to 'make clean' _before_ you switch branches. Otherwise,
>> you risk leaving some objects laying around. Since the script
>> runs 'nm' on all objects it finds, any stale ones can cause problems.
>> (Of course, I almost always forget, so I frequently have to manually
>> check for and remove stale objects!)
> 
> Gross. I would not be opposed to a Makefile rule that outputs the
> correct set of OBJECTS so this (or other) scripts could build on it.
> 
> IIRC, BSD make has an option to do this "make -V OBJECTS" or something,
> but I don't thnk there's an easy way to do so.

Hmm, I would go in the opposite direction and take a leaf out of
Ævar's book (see commit bc548efe) and this one-liner:

diff --git a/Makefile b/Makefile
index ee89c06..c08c25e 100644
--- a/Makefile
+++ b/Makefile
@@ -2506,7 +2506,7 @@ profile-clean:
 
 clean: profile-clean coverage-clean
$(RM) *.res
-   $(RM) $(OBJECTS)
+   $(RM) $(addsuffix *.o,$(object_dirs))
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)

This would actually solve my problem, but it actually isn't a
_complete_ solution. (Hint: think about what isn't in $(OBJECTS),
depending on the configuration). ;-)

> Or, since it seems to find useful results quite frequently, maybe it
> would be worth including the script inside git (and triggering it with
> an optional Makefile rule). It sounds like we'd need a way to annotate
> known false positives, but if it were in common use, it would be easier
> to get people to keep that list up to date.

Hmm, I suspect that wouldn't happen, which would reduce it usefulness
and ultimately lead to it not being used. (Updating the 'stop list' would
fast become a burden.)

I find it useful to flag these issues automatically, but I still need
to look at each symbol and decide what to do (you may not agree with
some of my choices either - take a look at the output on the master
branch!).

The way I use it, I effectively ignore the 'stop list' maintenance issues.

ATB,
Ramsay Jones

Re: [PATCH v10 13/14] convert: add filter..process option

[Junio C Hamano]

Torsten Bögershausen  writes:

>> +test_cmp_count () {
>> +expect=$1 actual=$2
>
> That could be 
> expect="$1"
> actual="$2"

Yes, but it does not have to ;-).

Re: What's cooking in git.git (Oct 2016, #03; Tue, 11)

[Junio C Hamano]

Johannes Schindelin  writes:

>> I'll mark it as "wait for follow-up fix" in whats-cooking.txt (on
>> 'todo' branch) to remind myself not to merge it yet.
>
> May I request your guidance as to your preference how to proceed?
> ...

I guess I didn't see this before I sent my response to the review
thread, which was in my pile of "these need more thought than others
before responding" topics.  

> Here are the options I see:
>
> A) remove the tests in question
>
> B) mark them as !MINGW instead
>
> C) change just those two tests from using `$PWD` (pseudo-Unix path) to
>   `$(pwd)` (native path)
>
> I would like to hear your feedback about your preference, but not without
> priming you a little bit by detailing my current opinion on the matter:
>
> While I think B) would be the easiest to read, C) would document the
> expected behavior better. A) would feel to me like shrugging, i.e. the
> lazy, wrong thing to do.
>
> What do you think?

As to my preference on tests, I guess what I suggested was a cross
between your B and C below, and I can go with either one as an
abbreviated version of my preference ;-) 

I am still wondering if the test is expecting the right behaviour,
though.  If some codepaths rely on a question "please resolve '../.'
relative to 'path/to/dir/.'" being answered as "that's path/to/dir
itself", it smells to me that the downstream of the dataflow that
expects such an answer, as well as the machinery that produces such
an answer, are acting as two wrongs that happen to cancel each
other.  Am I grossly misunderstanding what that test is doing?

Re: [PATCH v2 2/3] gitweb: Link to 7-char+ SHA1s, not only 8-char+

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> As far as I can tell the only outstanding "change this" is your
> s/SHA1/SHA-1/ in , do
> you want to fix that up or should I submit another series?

I think I did that already myself while queuing.  Could you fetch
what I queued on 'pu' to double check?

I think the diff between what was posted and what is queued (I just
checked) looks like this:

-gitweb: Link to 7-char+ SHA1s, not only 8-char+
+gitweb: link to 7-char+ SHA-1s, not only 8-char+

 Change the minimum length of an abbreviated object identifier in the
 commit message gitweb tries to turn into link from 8 hexchars to 7.
 
@@ -5,16 +12,18 @@
 SHA-1 in commit log message links to "object" view", 2006-12-10), but
 the default abbreviation length is 7, and has been for a long time.
 
-It's still possible to reference SHA1s down to 4 characters in length,
+It's still possible to reference SHA-1s down to 4 characters in length,
 see v1.7.4-1-gdce9648's MINIMUM_ABBREV, but I can't see how to make
 git actually produce that, so I doubt anyone is putting that into log
-messages in practice, but people definitely do put 7 character SHA1s
+messages in practice, but people definitely do put 7 character SHA-1s
 into log messages.
 
 I think it's fairly dubious to link to things matching [0-9a-fA-F]
 here as opposed to just [0-9a-f], that dates back to the initial
 version of gitweb from 161332a ("first working version",
-2005-08-07). Git will accept all-caps SHA1s, but didn't ever produce
+2005-08-07). Git will accept all-caps SHA-1s, but didn't ever produce
 them as far as I can tell.
 
 Signed-off-by: Ævar Arnfjörð Bjarmason 
+Acked-by: Jakub Narębski 
+Signed-off-by: Junio C Hamano 

Re: Merge conflicts in .gitattributes can cause trouble

[Johannes Schindelin]

Hi Lars,

On Tue, 4 Oct 2016, Lars Schneider wrote:

> If there is a conflict in the .gitattributes during a merge then it looks 
> like as if the attributes are not applied

I tried to replicate this behavior, to the point where I wrote a patch
that demonstrates the breakage so I could single-step in a debugger to
find out where things go wrong, and fix them.

Alas, I found out that the .gitattributes are read *before* any merge
conflict arises in the case you demonstrated. Which kind of makes sense,
because the gitattributes decide over which merge driver to use, among
other things.

So in your example:

> Consider this script on Windows:
> 
> $ git init .
> $ touch first.commit
> $ git add .
> $ git commit -m "first commit"
> 
> $ git checkout -b branch
> $ printf "*.bin binary\n" >> .gitattributes
> $ git add .
> $ git commit -m "tracking *.bin files"
> 
> $ git checkout master
> $ printf "binary\ndata\n" > file.dat # <-- Unix line ending!
> $ printf "*.dat binary\n" >> .gitattributes # <-- Tell Git to keep Unix line 
> ending!
> $ git add .
> $ git commit -m "tracking *.dat files"
> $ git cat-file -p :file.dat | od -c
> 000   b   i   n   a   r   y  \n   d   a   t   a  \n 
>   <-- Correct!
> $ git checkout branch

At this point, the .gitattributes list only .bin files as binary. That is
the revision of the .gitattributes used by this command:

> $ git merge master # <-- Causes merge conflict!

And as a consequence, the .gitattributes do not tell Git that it should
handle .dat files as binary. Which means that...

> $ printf "*.bin binary\n*.dat binary\n" > .gitattributes # <-- Fix merge 
> conflict!
> $ git add .
> $ git commit -m "merged"
> $ git cat-file -p :file.dat | od -c
> 000   b   i   n   a   r   y  \r  \n   d   a   t   a  \r  \n
>   <-- Wrong!

... this is actually expected! Why? Because the .gitattributes that were
in effect when the user asked to perform a merge said so.

If you adjust .gitattributes *before* merging `master`, it works as you
would expect: the line endings are not changed.

The reason to do it this way: we want to respect the .gitattributes as per
the current worktree. We go even so far that we respect uncommitted
changes to said file...

> Possible solutions:
> 
> 1. We could print an appropriate warning if we detect a merge conflict 
>in .gitattributes
> 
> 2. We could disable all line ending conversions in case of a merge conflict
>(I am not exactly sure about all the implications, though)
> 
> 3. We could salvage what we could of the .gitattributes file, 
>perhaps by using the version from HEAD (or more likely, the ours stage of
>the index) -- suggested by Peff on the related GitHub issue mentioned below

I would vote for:

4. We keep letting Git read in the *current* version of .gitattributes
   *before* the merge, and apply those attributes while performing the
   merge.

Ciao,
Dscho

RE: Uninitialized submodules as symlinks

[David Turner]

> -Original Message-
> From: Duy Nguyen [mailto:pclo...@gmail.com]
> Sent: Monday, October 17, 2016 5:46 AM
> To: David Turner
> Cc: Stefan Beller; git@vger.kernel.org
> Subject: Re: Uninitialized submodules as symlinks
> 
> On Sat, Oct 8, 2016 at 2:59 AM, David Turner 
> wrote:
> >
> >
> >> -Original Message-
> >> From: Stefan Beller [mailto:sbel...@google.com]
> >> Sent: Friday, October 07, 2016 2:56 PM
> >> To: David Turner
> >> Cc: git@vger.kernel.org
> >> Subject: Re: Uninitialized submodules as symlinks
> >>
> >> On Fri, Oct 7, 2016 at 11:17 AM, David Turner
> >> 
> >> wrote:
> >> > Presently, uninitialized submodules are materialized in the working
> >> > tree as empty directories.
> >>
> >> Right, there has to be something, to hint at the user that creating a
> >> file with that path is probably not what they want.
> >>
> >> >  We would like to consider having them be symlinks.  Specifically,
> >> > we'd like them to be symlinks into a FUSE filesystem which
> >> > retrieves files on demand.
> >> >
> >> > We've actually already got a FUSE filesystem written, but we use a
> >> > different (semi-manual) means to connect it to the initialized
> submodules.
> >>
> >> So you currently do a
> >>
> >> git submodule init 
> >> custom-submodule make-symlink 
> >>
> >> ?
> >
> > We do something like
> >
> > For each initialized submodule: symlink it into the right place in
> > .../somedir For each uninitialized submodule: symlink from the FUSE
> > into the right place in .../somedir
> >
> > So .../somedir has the structure of the git main repo, but is all
> symlinks -- some into FUSE, some into the git repo.
> >
> > This means that when we initialize (or deinitialize) a submodule, we
> need to re-run the linking script.
> 
> Do .git files work? If .git files point to somewhere in fuse, I guess you
> still have file retrieval on demand. It depends on what files to retrieve
> I guess. If you want worktree files, not object database then .git files
> won't work because worktree remains in the same filesystem as the super
> repo.

Yes, we want worktree files (or even worktree files + built artifacts).

RE: Uninitialized submodules as symlinks

[David Turner]

> -Original Message-
> From: Heiko Voigt [mailto:hvo...@hvoigt.net]
> Sent: Thursday, October 13, 2016 12:10 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: Uninitialized submodules as symlinks
> 
> On Fri, Oct 07, 2016 at 06:17:05PM +, David Turner wrote:
> > Presently, uninitialized submodules are materialized in the working
> > tree as empty directories.  We would like to consider having them be
> > symlinks.  Specifically, we'd like them to be symlinks into a FUSE
> > filesystem which retrieves files on demand.
> 
> How about portability? This feature would only work on Unix like operating
> systems. You have to be careful to not break Windows since they do not
> have symlinks.

Windows doesn't support FUSE either IIRC.  Since this would be an alternate 
mode of operation, Windows would still work fine on the old model.

[PATCH 1/4] i18n: apply: mark error message for translation

[Vasco Almeida]

Update test to reflect changes.

Signed-off-by: Vasco Almeida 
---
 apply.c   | 4 ++--
 t/t4254-am-corrupt.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/apply.c b/apply.c
index 8215874..705cf56 100644
--- a/apply.c
+++ b/apply.c
@@ -1586,8 +1586,8 @@ static int find_header(struct apply_state *state,
patch->new_name = xstrdup(patch->def_name);
}
if (!patch->is_delete && !patch->new_name) {
-   error("git diff header lacks filename 
information "
-"(line %d)", state->linenr);
+   error(_("git diff header lacks filename 
information "
+"(line %d)"), state->linenr);
return -128;
}
patch->is_toplevel_relative = 1;
diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh
index 9bd7dd2..168739c 100755
--- a/t/t4254-am-corrupt.sh
+++ b/t/t4254-am-corrupt.sh
@@ -31,7 +31,7 @@ test_expect_success 'try to apply corrupted patch' '
 test_expect_success 'compare diagnostic; ensure file is still here' '
echo "error: git diff header lacks filename information (line 4)" 
>expected &&
test_path_is_file f &&
-   test_cmp expected actual
+   test_i18ncmp expected actual
 '
 
 test_done
-- 
2.10.1.459.g5fd885d

[PATCH 2/4] i18n: convert mark error messages for translation

[Vasco Almeida]

Mark error messages about CRLF for translation.

Update test to reflect changes.

Signed-off-by: Vasco Almeida 
---
 convert.c   | 12 
 t/t0020-crlf.sh |  6 +-
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/convert.c b/convert.c
index 077f5e6..0ad39b1 100644
--- a/convert.c
+++ b/convert.c
@@ -197,17 +197,21 @@ static void check_safe_crlf(const char *path, enum 
crlf_action crlf_action,
 * CRLFs would not be restored by checkout
 */
if (checksafe == SAFE_CRLF_WARN)
-   warning("CRLF will be replaced by LF in %s.\nThe file 
will have its original line endings in your working directory.", path);
+   warning(_("CRLF will be replaced by LF in %s.\n"
+ "The file will have its original line"
+ " endings in your working directory."), path);
else /* i.e. SAFE_CRLF_FAIL */
-   die("CRLF would be replaced by LF in %s.", path);
+   die(_("CRLF would be replaced by LF in %s."), path);
} else if (old_stats->lonelf && !new_stats->lonelf ) {
/*
 * CRLFs would be added by checkout
 */
if (checksafe == SAFE_CRLF_WARN)
-   warning("LF will be replaced by CRLF in %s.\nThe file 
will have its original line endings in your working directory.", path);
+   warning(_("LF will be replaced by CRLF in %s.\n"
+ "The file will have its original line"
+ " endings in your working directory."), path);
else /* i.e. SAFE_CRLF_FAIL */
-   die("LF would be replaced by CRLF in %s", path);
+   die(_("LF would be replaced by CRLF in %s"), path);
}
 }
 
diff --git a/t/t0020-crlf.sh b/t/t0020-crlf.sh
index f94120a..71350e0 100755
--- a/t/t0020-crlf.sh
+++ b/t/t0020-crlf.sh
@@ -83,7 +83,11 @@ test_expect_success 'safecrlf: print warning only once' '
git add doublewarn &&
git commit -m "nowarn" &&
for w in Oh here is CRLFQ in text; do echo $w; done | q_to_cr 
>doublewarn &&
-   test $(git add doublewarn 2>&1 | grep "CRLF will be replaced by LF" | 
wc -l) = 1
+   git add doublewarn 2>err &&
+   if test_have_prereq C_LOCALE_OUTPUT
+   then
+   test $(grep "CRLF will be replaced by LF" err | wc -l) = 1
+   fi
 '
 
 
-- 
2.10.1.459.g5fd885d

[PATCH 4/4] i18n: diff: mark warnings for translation

[Vasco Almeida]

Mark rename_limit_warning and degrade_cc_to_c_warning and
rename_limit_warning for translation.

Signed-off-by: Vasco Almeida 
---
 diff.c | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/diff.c b/diff.c
index 1d304e0..1687317 100644
--- a/diff.c
+++ b/diff.c
@@ -4638,25 +4638,25 @@ static int is_summary_empty(const struct 
diff_queue_struct *q)
 }
 
 static const char rename_limit_warning[] =
-"inexact rename detection was skipped due to too many files.";
+N_("inexact rename detection was skipped due to too many files.");
 
 static const char degrade_cc_to_c_warning[] =
-"only found copies from modified paths due to too many files.";
+N_("only found copies from modified paths due to too many files.");
 
 static const char rename_limit_advice[] =
-"you may want to set your %s variable to at least "
-"%d and retry the command.";
+N_("you may want to set your %s variable to at least "
+   "%d and retry the command.");
 
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
 {
if (degraded_cc)
-   warning(degrade_cc_to_c_warning);
+   warning(_(degrade_cc_to_c_warning));
else if (needed)
-   warning(rename_limit_warning);
+   warning(_(rename_limit_warning));
else
return;
if (0 < needed && needed < 32767)
-   warning(rename_limit_advice, varname, needed);
+   warning(_(rename_limit_advice), varname, needed);
 }
 
 void diff_flush(struct diff_options *options)
-- 
2.10.1.459.g5fd885d

[PATCH 3/4] i18n: credential-cache--daemon: mark advice for translation

[Vasco Almeida]

Mark permissions_advice for translation.

Signed-off-by: Vasco Almeida 
---
 credential-cache--daemon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 1e5f16a..46c5937 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -219,11 +219,11 @@ static void serve_cache(const char *socket_path, int 
debug)
close(fd);
 }
 
-static const char permissions_advice[] =
+static const char permissions_advice[] = N_(
 "The permissions on your socket directory are too loose; other\n"
 "users may be able to read your cached credentials. Consider running:\n"
 "\n"
-"  chmod 0700 %s";
+"  chmod 0700 %s");
 static void init_socket_directory(const char *path)
 {
struct stat st;
@@ -232,7 +232,7 @@ static void init_socket_directory(const char *path)
 
if (!stat(dir, )) {
if (st.st_mode & 077)
-   die(permissions_advice, dir);
+   die(_(permissions_advice), dir);
} else {
/*
 * We must be sure to create the directory with the correct 
mode,
-- 
2.10.1.459.g5fd885d

Re: Automagic `git checkout branchname` mysteriously fails

[Duy Nguyen]

On Sat, Oct 15, 2016 at 4:06 AM, Martin Langhoff
 wrote:
> On Fri, Oct 14, 2016 at 4:58 PM, Kevin Daudt  wrote:
>> Correct, this only works when it's unambiguous what branch you actually
>> mean.
>
> That's not surprising, but there isn't a warning. IMHO, finding
> several branch matches is a strong indication that it'll be worth
> reporting to the user that the DWIM machinery got hits, but couldn't
> work it out.
>
> I get that process is not geared towards making a friendly msg easy,
> but we could print to stderr something like:
>
>  "branch" matches more than one candidate ref, cannot choose automatically.
>  If you mean to check out a branch, try git branch command.
>  If you mean to check out a file, use -- before the pathname to
>  disambiguate.

Or even better, list all ambiguous candidates like Jeff did for
ambiguous short SHA-1 in 1ffa26c (get_short_sha1: list ambiguous
objects on error - 2016-09-26).There were a few occasions I was
confused by ambiguous refs and displaying them all would help me see
what problem was much faster.
-- 
Duy

Re: [RFC] Case insensitive Git attributes

[Duy Nguyen]

On Mon, Oct 17, 2016 at 5:46 PM, Johannes Schindelin
 wrote:
> Hi Duy,
>
> On Mon, 17 Oct 2016, Duy Nguyen wrote:
>
>> On Mon, Oct 17, 2016 at 3:57 PM, Johannes Schindelin
>>  wrote:
>> > Hi Stefan,
>> >
>> > On Sun, 16 Oct 2016, Stefan Beller wrote:
>> >
>> >> Conceptually I would prefer if we had a single switch that indicates a
>> >> case insensitive FS.
>> >
>> > AFAIU Lars' use case is where the FS is *case sensitive*, but he still
>> > needs the .gitattributes to be *case insensitive* because that file
>> > originates from a developer with such a file system.
>> >
>> > Otherwise he would simply tack onto the core.ignoreCase flag.
>>
>> That sounds to me like setting core.ignoreCase to true (on all devs'
>> repo) would "solve" this.
>
> It is good that you quoted this verb, because it does not solve things.
> Instead, it would try to use the flag for two slightly incompatible
> purposes at the same time.
>
> The first (and so far, only) purpose is to tell Git that the current file
> system is case insensitive.
>
> The new purpose you described would be to tell Git that the *user* does
> not care about the file names' case, even if the file system does.
>
> I do not think that this leads to a better situation than before. Instead,
> I am convinced that it will cause new and sometimes "entertaining"
> problems because you can no longer discern between those two purposes
> based on core.ignoreCase, you would have to teach Git to test every single
> time whether the file system is case-sensitive or not.

I agree. Which is why I wrote "we probably want something in the same
spirit but limited to .gitattributes and .gitignore only". In other
words we could have core.someName that makes .gitattributes and
.gitignore patterns case-insensitive (or core-sensitive). If it's
present, it overrides core.ignoreCase. If it's not present,
core.ignoreCase decides. I'm just not sure if the new config should
cover everything involving filename's case in git. That's too big to
fit in my head.

> Needless to say, I'd rather not see that happening. Many users, including
> my colleagues and myself, rely on Git being a rock solid piece of
> software, and that change would make it less so.
-- 
Duy

Re: [RFC] Case insensitive Git attributes

[Johannes Schindelin]

Hi Duy,

On Mon, 17 Oct 2016, Duy Nguyen wrote:

> On Mon, Oct 17, 2016 at 3:57 PM, Johannes Schindelin
>  wrote:
> > Hi Stefan,
> >
> > On Sun, 16 Oct 2016, Stefan Beller wrote:
> >
> >> Conceptually I would prefer if we had a single switch that indicates a
> >> case insensitive FS.
> >
> > AFAIU Lars' use case is where the FS is *case sensitive*, but he still
> > needs the .gitattributes to be *case insensitive* because that file
> > originates from a developer with such a file system.
> >
> > Otherwise he would simply tack onto the core.ignoreCase flag.
> 
> That sounds to me like setting core.ignoreCase to true (on all devs'
> repo) would "solve" this.

It is good that you quoted this verb, because it does not solve things.
Instead, it would try to use the flag for two slightly incompatible
purposes at the same time.

The first (and so far, only) purpose is to tell Git that the current file
system is case insensitive.

The new purpose you described would be to tell Git that the *user* does
not care about the file names' case, even if the file system does.

I do not think that this leads to a better situation than before. Instead,
I am convinced that it will cause new and sometimes "entertaining"
problems because you can no longer discern between those two purposes
based on core.ignoreCase, you would have to teach Git to test every single
time whether the file system is case-sensitive or not.

Needless to say, I'd rather not see that happening. Many users, including
my colleagues and myself, rely on Git being a rock solid piece of
software, and that change would make it less so.

Ciao,
Dscho

Re: [PATCH v1 1/2] config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11

[Jeff King]

On Sun, Oct 16, 2016 at 05:25:49PM -0700, larsxschnei...@gmail.com wrote:

> From: Lars Schneider 
> 
> Apple removed the OpenSSL header files in macOS 10.11 and above. OpenSSL
> was deprecated since macOS 10.7.
> 
> Set `NO_OPENSSL` and `APPLE_COMMON_CRYPTO` to `YesPlease` as default for
> macOS. Make it possible to override this and use OpenSSL by defining
> `DARWIN_OPENSSL`.

I like that you gave an override, but I don't think it works in all
cases:

> diff --git a/config.mak.uname b/config.mak.uname
> index b232908..f0c94a9 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -108,6 +108,12 @@ ifeq ($(uname_S),Darwin)
>   ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 11 
> && echo 1),1)
>   HAVE_GETDELIM = YesPlease
>   endif
> + ifeq ($(shell test "`expr "$(uname_R)" : '\([0-9][0-9]*\)\.'`" -ge 15 
> && echo 1),1)
> + ifndef DARWIN_OPENSSL
> + NO_OPENSSL = YesPlease
> + APPLE_COMMON_CRYPTO=YesPlease
> + endif
> + endif

This is in config.mak.uname, which gets sourced before config.mak (and
ifndef is evaluated at the time of parsing). So it would work to do:

  make DARWIN_OPENSSL=Yep

but not:

  echo DARWIN_OPENSSL=Yep >>config.mak
  make

I think you'd have to set a flag in config.mak.uname, and then resolve
it in the Makefile proper like:

  ifdef DARWIN_OPENSSL
# Overrides AUTO_AVOID_OPENSSL, do nothing.
  else ifdef AUTO_AVOID_OPENSSL
NO_OPENSSL = YesPlease
APPLE_COMMON_CRYPTO = YesPlease
  endif

but that's totally untested.

-Peff

Re: Uninitialized submodules as symlinks

[Duy Nguyen]

On Sat, Oct 8, 2016 at 2:59 AM, David Turner  wrote:
>
>
>> -Original Message-
>> From: Stefan Beller [mailto:sbel...@google.com]
>> Sent: Friday, October 07, 2016 2:56 PM
>> To: David Turner
>> Cc: git@vger.kernel.org
>> Subject: Re: Uninitialized submodules as symlinks
>>
>> On Fri, Oct 7, 2016 at 11:17 AM, David Turner 
>> wrote:
>> > Presently, uninitialized submodules are materialized in the working tree
>> > as empty directories.
>>
>> Right, there has to be something, to hint at the user that creating a file
>> with that path is probably not what they want.
>>
>> >  We would like to consider having them be symlinks.  Specifically, we'd
>> > like them to be symlinks into a FUSE filesystem which retrieves files on
>> > demand.
>> >
>> > We've actually already got a FUSE filesystem written, but we use a
>> > different (semi-manual) means to connect it to the initialized submodules.
>>
>> So you currently do a
>>
>> git submodule init 
>> custom-submodule make-symlink 
>>
>> ?
>
> We do something like
>
> For each initialized submodule: symlink it into the right place in .../somedir
> For each uninitialized submodule: symlink from the FUSE into the right place 
> in .../somedir
>
> So .../somedir has the structure of the git main repo, but is all symlinks -- 
> some into FUSE, some into the git repo.
>
> This means that when we initialize (or deinitialize) a submodule, we need to 
> re-run the linking script.

Do .git files work? If .git files point to somewhere in fuse, I guess
you still have file retrieval on demand. It depends on what files to
retrieve I guess. If you want worktree files, not object database then
.git files won't work because worktree remains in the same filesystem
as the super repo.
-- 
Duy

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

[Jeff King]

On Mon, Oct 17, 2016 at 11:04:19AM +0200, Johannes Schindelin wrote:

> > Gross. I would not be opposed to a Makefile rule that outputs the
> > correct set of OBJECTS so this (or other) scripts could build on it.
> 
> You could also use the method I use in Git for Windows to "extend" the
> Makefile:
> 
> -- snipsnap --
> cat >dummy.mak < include Makefile
> 
> blub: $(OBJECTS)
>   do-something-with $^
> EOF
> 
> make -f dummy.mak blub

Hacky but clever. I like it.

In the particular case of git, I think I've cheated similarly before by
putting things in config.mak, though of course an arbitrary script can't
assume it can overwrite that file.

-Peff

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

[Jeff King]

[tl;dr: the version in your repo is fine, and there's a trivial fix
 below if we want to silence the warning in the meantime]

On Mon, Oct 17, 2016 at 10:37:52AM +0200, Johannes Schindelin wrote:

> > I'm not sure I agree. IIRC, Assigning values outside the range of an enum 
> > has
> > always been fishy according to the standard, and a compiler really is
> > allowed to allocate a single bit for storage for this enum.
> 
> Really? I did see my share of code that completely violated this freedom,
> by assuming that it was really okay to cast a -1 to an enum and then test
> for that value later, when -1 was not a legal enum value.

I poked around a bit, and it seems we're both half-wrong. C99 says:

  6.7.2.2 Enumeration specifiers
  [...]
  The expression that defines the value of an enumeration constant shall
  be an integer constant expression that has a value representable as an
  int.
  [...]
  Each enumerated type shall be compatible with char, a signed integer
  type, or an unsigned integer type. The choice of type is
  implementation-defined, but shall be capable of representing the
  values of all the members of the enumeration.

My reading is that it can't be a single-bit bitfield as I claimed, but
it also isn't necessarily interchangeable with an int. But you get at
least a "char", and you can use all of those integer values even if they
aren't explicitly part of the set. And I'd assume that goes for values
even beyond the largest tag as long as you don't need more bits, so
that:

  enum { A = 1, B = 2, C = 4 } x = A | B | C;

is OK (though I didn't see anything particularly about that in the
standard).

Assigning "-1" works in the same way that normal "unsigned x = -1" works
(and is defined by the standard), though of course it may unexpectedly
conflict with an actual enum value if the compiler chooses a smaller
type (e.g., it may literally be 255 in many cases).

Anyway. Enough language lawyering. It seems like clang is being overly
strict in its interpretation of the standard (it should be giving us at
least a char's worth of values). But it matters less what the standard
says and more what real compilers do, and we have to deal with clang's
behavior.

> In any case, the fact that even one compiler used to build Git *may*
> violate that standard, and that we therefore need such safety guards as
> the one under discussion, still makes me think that this warning, while
> certainly well-intentioned, is poison for cross-platform projects.

Oh, I agree that the warning is annoying, and the code should not go
away. We just need to figure out how to silence clang.

> > I'm happy to test the TODO_NOOP version against clang (and prepare a
> > patch on top if it still complains), but that doesn't seem to have
> > Junio's tree at all yet.
> 
> Junio chose to pick up only one patch series out of the rebase--helper
> thicket at a time, it seems. I did send out at least one revision per
> patch series prior to integrating them into Git for Windows v2.10.0,
> though. Plus, I kept updating the `interactive-rebase` branch in my
> repository on GitHub (https://github.com/dscho/git).

Thanks, I was able to test that branch. It looks like clang is happy
with it because you compare against the max value. Unlike the
ARRAY_SIZE() check, this does mean if somebody modifies the enum without
touching the array, we might go out of bounds. But things would be
severely broken enough from the mismatch that I don't think it's worth
worrying about too much (and I see you have a nice comment warning
people about this).

If the rest of your interactive-rebase branch is coming soon, I think we
can probably ignore it for now. Otherwise something like:

diff --git a/sequencer.c b/sequencer.c
index d662c6b..1fdc35e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -620,7 +620,8 @@ static int allow_empty(struct replay_opts *opts, struct 
commit *commit)
 
 enum todo_command {
TODO_PICK = 0,
-   TODO_REVERT
+   TODO_REVERT,
+   TODO_MAX
 };
 

is probably the simplest portable fix.

As a more clever change, I wondered if switching the enum values from
(0,1) to (1,2) would silence the warning, and indeed it does. Which I
assume is because using bit-flags, we could now represent "1|2", or "3",
which is larger than the array (well, obviously "2" is, but we'd need to
subtract 1 when indexing the array). I don't think that's a good route,
though, because it loses the 0-indexing, the benefits of
zero-initialization, etc. I was mostly just poking at how clang
perceives the enum values.

> P.S.: I cannot wait for the day when somebody with an artistic touch
> provides .css for the public-inbox.org site so it stops threatening
> causing eye cancer to me.

Heh. I gently hinted something similar to Eric in the past, but I think
he actually likes how it looks. He has invited others to mirror
public-inbox and make their own interface, though. I just lack the
"artistic touch" you mentioned.

-Peff

Re: Uninitialized submodules as symlinks

[Heiko Voigt]

On Fri, Oct 14, 2016 at 09:48:16AM -0700, Junio C Hamano wrote:
> Kevin Daudt  writes:
> 
> > On Thu, Oct 13, 2016 at 06:10:17PM +0200, Heiko Voigt wrote:
> >> On Fri, Oct 07, 2016 at 06:17:05PM +, David Turner wrote:
> >> > Presently, uninitialized submodules are materialized in the working
> >> > tree as empty directories.  We would like to consider having them be
> >> > symlinks.  Specifically, we'd like them to be symlinks into a FUSE
> >> > filesystem which retrieves files on demand.
> >> 
> >> How about portability? This feature would only work on Unix like
> >> operating systems. You have to be careful to not break Windows since
> >> they do not have symlinks.
> >
> > NTFS does have symlinks, but you need admin right to create them though
> > (unless you change the policy).
> 
> That sounds like saying "It has, but it practically is not usable by
> Git as a mechanism to achieve this goal" to me.

Yes and that is why Git for Windows does not use them and I simplified
to: "Windows does not have symlinks". For a normal user there is no such
thing as symlinks on Windows, unfortunately.

Cheers Heiko

Re: [RFC] Case insensitive Git attributes

[Duy Nguyen]

On Mon, Oct 17, 2016 at 3:57 PM, Johannes Schindelin
 wrote:
> Hi Stefan,
>
> On Sun, 16 Oct 2016, Stefan Beller wrote:
>
>> Conceptually I would prefer if we had a single switch that indicates a
>> case insensitive FS.
>
> AFAIU Lars' use case is where the FS is *case sensitive*, but he still
> needs the .gitattributes to be *case insensitive* because that file
> originates from a developer with such a file system.
>
> Otherwise he would simply tack onto the core.ignoreCase flag.

That sounds to me like setting core.ignoreCase to true (on all devs'
repo) would "solve" this. Yes core.ignoreCase may introduce some side
effects when used on case-sensitive filesystems, so we probably want
something in the same spirit but limited to .gitattributes and
.gitignore only.
-- 
Duy

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

[Johannes Schindelin]

Hi Ramsay & Peff,

On Sun, 16 Oct 2016, Jeff King wrote:

> On Mon, Oct 17, 2016 at 02:37:58AM +0100, Ramsay Jones wrote:
> 
> > Hmm, well, you have to remember that 'make clean' sometimes doesn't
> > make clean. Ever since the Makefile was changed to only remove
> > $(OBJECTS), rather than *.o xdiff/*.o etc., you have to remember to
> > 'make clean' _before_ you switch branches. Otherwise, you risk leaving
> > some objects laying around. Since the script runs 'nm' on all objects
> > it finds, any stale ones can cause problems.  (Of course, I almost
> > always forget, so I frequently have to manually check for and remove
> > stale objects!)
> 
> Gross. I would not be opposed to a Makefile rule that outputs the
> correct set of OBJECTS so this (or other) scripts could build on it.

You could also use the method I use in Git for Windows to "extend" the
Makefile:

-- snipsnap --
cat >dummy.mak <

Re: [RFC] Case insensitive Git attributes

[Johannes Schindelin]

Hi Stefan,

On Sun, 16 Oct 2016, Stefan Beller wrote:

> Conceptually I would prefer if we had a single switch that indicates a
> case insensitive FS.

AFAIU Lars' use case is where the FS is *case sensitive*, but he still
needs the .gitattributes to be *case insensitive* because that file
originates from a developer with such a file system.

Otherwise he would simply tack onto the core.ignoreCase flag.

Ciao,
Dscho

Re: [RFC] Case insensitive Git attributes

[Johannes Schindelin]

Hi Lars,

On Sun, 16 Oct 2016, Lars Schneider wrote:

> One idea could be to add an attribute "case-sensitive" (or
> "caseSensitive") and set it to false (if desired) for all files in
> .gitattributes for a given repo.
> 
> ### .gitattributes example ###
> 
> * case-sensitive=false
> *.bar something
> 
> ###

Hrm. Maybe a better idea would be to warn when attributes match a file
name with a different case?

Ciao,
Dscho

Re: [PATCH v3 07/25] sequencer: completely revamp the "todo" script parsing

[Johannes Schindelin]

Hi Peff,

On Sun, 16 Oct 2016, Jeff King wrote:

> On Sun, Oct 16, 2016 at 10:09:12AM +0200, Johannes Schindelin wrote:
> 
> > > Good catch. It technically needs to check the lower bound, too. In
> > > theory, if somebody wanted to add an enum value that is negative, you'd
> > > use a signed cast and check against both 0 and ARRAY_SIZE(). In
> > > practice, that is nonsense for this case, and using an unsigned type
> > > means that any negative values become large, and the check catches them.
> > 
> > I am pretty certain that I disagree with that warning: enums have been
> > used as equivalents of ints for a long time, and will be, for a long time
> > to come.
> 
> I'm not sure I agree. IIRC, Assigning values outside the range of an enum has
> always been fishy according to the standard, and a compiler really is
> allowed to allocate a single bit for storage for this enum.

Really? I did see my share of code that completely violated this freedom,
by assuming that it was really okay to cast a -1 to an enum and then test
for that value later, when -1 was not a legal enum value.

In any case, the fact that even one compiler used to build Git *may*
violate that standard, and that we therefore need such safety guards as
the one under discussion, still makes me think that this warning, while
certainly well-intentioned, is poison for cross-platform projects.

> > Given that this test is modified to `if (command < TODO_NOOP)` later, I
> > hope that you agree that it is not worth the trouble to appease that
> > compiler overreaction?
> 
> I don't mind if there are transient warnings on some compilers in the
> middle of a series, but I'm not sure when "later" is. The tip of "pu"
> has this warning right now when built with clang.

"Later" is the sequencer-i patch series I already sent out for review
[*1*], in particular the patch titled "sequencer (rebase -i):
differentiate between comments and 'noop'" [*2*].

> I'm happy to test the TODO_NOOP version against clang (and prepare a
> patch on top if it still complains), but that doesn't seem to have
> Junio's tree at all yet.

Junio chose to pick up only one patch series out of the rebase--helper
thicket at a time, it seems. I did send out at least one revision per
patch series prior to integrating them into Git for Windows v2.10.0,
though. Plus, I kept updating the `interactive-rebase` branch in my
repository on GitHub (https://github.com/dscho/git).

Ciao,
Dscho

Footnote *1*:
https://public-inbox.org/git/cover.1472633606.git.johannes.schinde...@gmx.de/

Footnote *2*:
https://public-inbox.org/git/736bcb8e860c876e32e8f89f68b0b901abedc187.1472633606.git.johannes.schinde...@gmx.de/t/#u

P.S.: I cannot wait for the day when somebody with an artistic touch
provides .css for the public-inbox.org site so it stops threatening
causing eye cancer to me.

Re: [PATCH 1/2] submodule: ignore trailing slash on superproject URL

[Junio C Hamano]

Johannes Schindelin  writes:

>> > expecting success:
>> > actual=$(git submodule--helper resolve-relative-url-test 
>> > '(null)' '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' '../.') &&
>> > test "$actual" = 'C:/git-sdk-64/usr/src/git/wip/t/trash 
>> > directory.t0060-path-utils/.'
>> >
>> > +++ git submodule--helper resolve-relative-url-test '(null)' 
>> > '/usr/src/git/wip/t/trash directory.t0060-path-utils/.' ../.
>> > ++ actual=C:/git-sdk-64/usr/src/git/wip/t/.
>> > ++ test C:/git-sdk-64/usr/src/git/wip/t/. = 
>> > 'C:/git-sdk-64/usr/src/git/wip/t/trash directory.t0060-path-utils/.'

This may well be total misunderstanding on my side, but is the
expectation of this test even correct?  If it wants to take "../."
relative to "$LEAD/t/trash utils/.", should't it go one level up
with ".." to $LEAD/t and then stay there with ".", expecting
"$LEAD/t" which is what the above is giving us?

IOW, the above makes me wonder why having one of these as the base

A - path/to/dir
B - path/to/dir/
C - path/to/dir/.

to resolve the relative "../." give different results.  Whether bash
on Windows removes the dot at the end of C to turn it into B, as
long as A and B give us the same result we wouldn't be hitting the
problem, no?

>> >  test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" 
>> > "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
>> >  test_submodule_relative_url "(null)" "$PWD/super_update_r2" 
>> > "../subsuper_update_r" "$(pwd)/subsuper_update_r"
>> > -test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
>> > +test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."

>> > The reasons this is ugly: we specifically test for *Unixy* paths when we
>> > use $PWD, as opposed to *Windowsy* paths when using $(pwd).

Just to ensure I am following this correctly, two tests that come
before the one you are touching above have $PWD on the input side
and $(pwd) on the expectation side.  That is what you mean by the
next paragraph, right?  They want to make sure that you honor the
Unixy user input on Windows and still produce Windowsy result, that
is.

>> > We do this to
>> > ensure a certain level of confidence that running things such as
>> >
>> > git clone --recurse-submodules /z/project/.
>> >
>> > work. And now that does not work anymore.

And I agree from that point of view that having to spell both sides
as $(pwd) would mean you are not testing that "Unixy input to
Windowsy output" expectation, but at the same time, I think you
would want "Windowsy input to Windowsy output" combination also does
produce correct result, which is not tested in the three tests shown
above.  IOW, probably you would want to test both (at least on any
platform where $PWD and $(pwd) textually disagree) for all these
[*1*], and the pair

"../." taken relative to "$(pwd)/." must be "$(pwd)/."
"../." taken relative to "$PWD/." must be "$(pwd)/."

test, because of the limitation of your bash, cannot have the latter
half of the pair, so you'd need to comment it out with in-code
explanation, perhaps?  IOW something along the lines of...

 -- >8 -- snip -- >8 --

test_submodule_relative_url "(null)" "$(pwd)/subsuper_update_r" 
"../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$(pwd)/super_update_r2" 
"../subsuper_update_r" "$(pwd)/subsuper_update_r"
test_submodule_relative_url "(null)" "$(pwd)/." "../." "$(pwd)/."

if test_have_prereq MINGW
then

test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" 
"../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" 
"../subsuper_update_r" "$(pwd)/subsuper_update_r"
# This does not work correctly because Win-Bash strips . at the end
# "of $PWD/."
# test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."

fi

 -- >8 -- snip -- >8 --

In any case, I find it more disturbing that we somehow ended up with
a system where these three things are expected to behave differently:

A - path/to/dir
B - path/to/dir/
C - path/to/dir/.

Is that something we can fix?


[Footnote]

*1* It is tempting to update the above test sequence using
a helper like:

tsru () {
test_submodule_relative_url "(null)" "$(pwd)/$1" "$2" "$(pwd)/$3"
if test_have_prereq MINGW
then
test_submodule_relative_url "(null)" "$PWD/$1" "$2" "$(pwd)/$3"
fi
}

then write the above three tests like so:

tsru subsuper_update_r ../subsubsuper_update_r subsubsuper_update_r
tsru super_update_r2 ../subsuper_update_r subsuper_update_r
tsru . ../. .

but you would want to disable the MINGW half for only the third
test, we cannot quite do that.