Re: [PATCH v7 01/11] dir: free untracked cache when removing it

[Christian Couder]

On Mon, Jan 25, 2016 at 8:16 PM, Stefan Beller  wrote:
> On Sun, Jan 24, 2016 at 7:28 AM, Christian Couder
>  wrote:
>> Signed-off-by: Christian Couder 
>> ---
>>  builtin/update-index.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index 7431938..a6fff87 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, 
>> const char *prefix)
>> add_untracked_ident(the_index.untracked);
>> the_index.cache_changed |= UNTRACKED_CHANGED;
>> } else if (!untracked_cache && the_index.untracked) {
>> +   free_untracked_cache(the_index.untracked);
>
> Do we need to free its members, too? (Or is it empty enough here,
> that there are no memleaks in there? If this were the case a hint in
> the commit message would be helpful)

free_untracked_cache() takes care of freeing the members of the struct
it is passed. If it doesn't free them all it's probably a bug. Many
free_*() functions in the Git code base do the same thing. And yeah I
think it is safer to free everything.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/11] config: add core.untrackedCache

[Christian Couder]

On Mon, Jan 25, 2016 at 9:47 PM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> diff --git a/read-cache.c b/read-cache.c
>> index 5be7cd1..a04ec8c 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1497,10 +1497,23 @@ static struct cache_entry *create_from_disk(struct 
>> ondisk_cache_entry *ondisk,
>>   return ce;
>>  }
>>
>> -static void check_ce_order(struct index_state *istate)
>> +static void post_read_index_from(struct index_state *istate)
>>  {
>>   unsigned int i;
>>
>> + switch (git_config_get_untracked_cache()) {
>> + case -1: /* keep: do nothing */
>> + break;
>> + case 0: /* false */
>> + remove_untracked_cache(istate);
>> + break;
>> + case 1: /* true */
>> + add_untracked_cache(istate);
>> + break;
>> + default: /* unknown value: do nothing */
>> + break;
>> + }
>> +
>>   for (i = 1; i < istate->cache_nr; i++) {
>>   struct cache_entry *ce = istate->cache[i - 1];
>>   struct cache_entry *next_ce = istate->cache[i];
>
> Bad manners.
>
>  * The new code added to an existing function, unless there is a
>good reason, goes to the bottom.  In this case, the verification
>of the ordering of cache entries and tweaking of UC extension are
>two unrelated things that can be independently done, and there is
>no justification why the new code has to come to top.
>
>  * The old function name served as a good documentation of what it
>does.  That is no longer the case.  Each unrelated segment of
>this new function needs to be commented.  Even better, perhaps
>leave the original check_ce_order() as-is, introduce a new
>function tweak_uc_extension(), and make the post_read_index()
>to be just two-liner function:
>
> static void post_read_index(struct index_state *istate)
> {
> check_ce_order(istate);
> tweak_uc_extension(istate);
> }
>
>That way the documentation value of each function that does one
>specific thing and named specific to its task will be kept, and
>there is no need for extra comments.

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

Re: [PATCH 2/2] Fix up the arguments for git stash.

[Paul Wagland]

Fair enough. I'll re-roll the patches with improved comments, and get these out 
to the list today. 

Cheers,
Paul

Sent from my iPhone

> On 26 Jan 2016, at 00:21, Junio C Hamano  wrote:
> 
> Paul Wagland  writes:
> 
>> Signed-off-by: Paul Wagland 
>> ---
> 
> This needs a better explanation than just "Fix up" in the title.
> What is broken in the current behaviour and what is the more desired
> behaviour?
> 
> Thanks.

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

Re: [PATCH 2/2] tag: do not show ambiguous tag names as "tags/foo"

[Karthik Nayak]

On Tue, Jan 26, 2016 at 5:34 AM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
>> git-tag has started showing tags with ambiguous names (i.e.,
>> when both "heads/foo" and "tags/foo" exists) as "tags/foo"
>> instead of just "foo". This is both:
>>
>>   - pointless; the output of "git tag" includes only
>> refs/tags, so we know that "foo" means the one in
>> "refs/tags".
>>
>> and
>>
>>   - ambiguous; in the original output, we know that the line
>> "foo" means that "refs/tags/foo" exists. In the new
>> output, it is unclear whether we mean "refs/tags/foo" or
>> "refs/tags/tags/foo".
>>
>> The reason this happens is that commit b7cc53e9 switched
>> git-tag to use ref-filter's "%(refname:short)" output
>> formatting, which was adapted from for-each-ref.
>> ...
>
> Karthik, getting a fix in for "git tag" regression is more important
> than the topics parked in 'pu', so I'll queue this patch in the
> early part of 'pu'.
>
> I personally feel that "refname:strip=" would be a good mechanism
> for end users to specify a custom format, and it is unclear to me
> what should happen when there are not enough elements to be
> stripped, so I do not think we want to cast the "we will show the
> whole thing" decision in stone prematurely only because we want to
> push out the regression fix soon.  So I may ask Jeff to rework this
> one (or I may end up trying to do so myself) not to squat on the
> nice strip= notation.  refname:strip-standard-prefix that removes
> the known prefix ("refs/heads", "refs/remotes" and "refs/tags") if
> present and does not touch the refname otherwise would leave us more
> time to decide what strip= should do in the error case.
>
> Unfortunately, this means kn/ref-filter-atom-parsing topic from you
> that were parked on 'pu' must be ejected for now, as any change in
> this area overlaps with it, and the atom parsing code would need to
> be updated to learn about the new attribute of the 'refname' atom
> (be it 'remove-prefix=', 'strip=', or something else) that
> we would decide to use for the regression fix anyway.

That should be fine, there are still changes to be done there so I'll rebase
on this and send that series.

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

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Christian Couder]

On Tue, Jan 5, 2016 at 9:03 AM, Karthik Nayak  wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=,position=)".
>
> Add Documetation and tests for the same.

s/Documetation/Documentation/

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

Re: [PATCH 17/19] mingw: fix git-svn tests that expect chmod to work

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > From: 마누엘 
> >
> > Some git-svn tests expect that the executable bit of files can be
> > toggled. On Windows, this is not possible because Windows' Access
> > Control Lists are much more fine-grained than the POSIX permission
> > concept. Let's just not try to flip the executable bit.
> 
> Most of the changes are protected by !POSIXPERM but one of them
> seems to use MINGW, which looks inconsistent and I suspect was not a
> distinction made on purpose.  The above description sounds to me
> that the !POSIXPERM prerequisite is the right thing to use.

This is my fault: there are two MINGW prereqs, and they guard against
trying to work with file names ending in a dot (which is illegal on
Windows' file systems).

My original plan was to split the patch into two, but I actually reworked
the patch from scratch.

> I am not sure if it is a good idea to sprinkle test-have-prereq and
> make the test script test different things on different platforms,
> though.

I agree that this is not desirable, and I changed it where possible to
simply skip the entire test case. In some cases, however, the 'setup' test
case was affected, and of course we cannot skip that one, else everything
falls apart.

The result of my work consists of these three patches (which will be part
of v2):

https://github.com/dscho/git/compare/ea813597~3...ea813597

Thanks,
Dscho

[BUG] typo DWIMery with alias broken (cd to random dir)

[Michael J Gruber]

Hi there,

with current next (989ee58 plus local additions) it seems that typo
DWIMery with aliases is broken, see below.

It appears that the typo DWIMery is broken only when there is a unique
automatic DWIM substitution for a mistyped alias.

I haven't bisected yet, but I suspect this to be related to recent
changes regarding the environment in which commands/aliases are started
(though this happens without extra work trees), so I'm cc'ing an expert
in that area. Funny, though, that my user name shows up...

I think the reason is that git.c's handle_alias() (or something else)
calls restore_env() multiple times, and restore_env frees orig_cwd such
that subsequent restore_env(0) with external_alias=0 tries to cd to a
random location.

I have no idea whether orig_cwd=0 after freeing or something else would
be the proper fix.

Michael

LANG=C git sss
WARNING: You called a Git command named 'sss', which does not exist.
Continuing under the assumption that you meant 'ss'
in 2.0 seconds automatically...
fatal: could not move to g...@drmicha.warpmail.net: No such file or directory
[mjg@skimbleshanks git]✗ LANG=C git ss
## HEAD (no branch)
?? a
?? a.patch
?? c2d.sh
[mjg@skimbleshanks git]✓ LANG=C git statu -sb
git: 'statu' is not a git command. See 'git --help'.

Did you mean one of these?
status
stage
stash
[mjg@skimbleshanks git]✗ LANG=C git statuss -sb
WARNING: You called a Git command named 'statuss', which does not exist.
Continuing under the assumption that you meant 'status'
in 2.0 seconds automatically...
## HEAD (no branch)
?? a
?? a.patch
?? c2d.sh
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 13/19] mingw: outsmart MSYS2's path substitution in t1508

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jan 2016, Junio C Hamano wrote:

> Eric Sunshine  writes:
> 
> > On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano  wrote:
> >> The new test hardcodes and promises such an incompatible behaviour,
> >> i.e. a request to create "@//b" results in "@/b" created, only to
> >> users on MINGW, fracturing the expectations of the Git userbase.
> >
> > What the commit message doesn't explain is that ...
> > ...
> > This commit message is trying to say that MSYS shell undesirably sees
> > @/fish as an absolute path, thus tries translating it to a Windows
> > path, such as @C:\fish. The only way to suppress this unwanted
> > translation is to manually double the slash, hence the patch makes the
> > test use @//fish which, when finally seen by the program, is just
> > @/fish, as was intended in the first place. So, doubling the slash on
> > MINGW is not promising incompatible behavior for MINGW users; it's
> > just working around unwanted path translation of the shell.
> 
> Ah, OK, thanks for clarifying it.  Presumably you would then use
> "checkout @//b" to switch to it, and "log @//b" to look at its
> hsitory.  When you read "git branch" output and see "@/b" in it, you
> would also not complain thinking "oh I thought I created "@//b", not
> with a single branch!".
> 
> Then no issues on allowing "checkout -b @//b" to create a branch
> "@/b" from me.

I actually disabled the test for MINGW instead, as I agree that we do not
want to test MSYS2 in our regression tests.

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

Re: [PATCH 18/19] mingw: skip a couple of git-svn tests that cannot pass on Windows

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Some of the tests expect that executable bits can be toggled, others
> > expect that paths are POSIX paths (but when they come through git.exe,
> > they are converted into Windows paths and necessarily differ), yet
> > others expect symbolic links to be available.
> 
> These skip the tests that cannot possibly pass in their entirety by
> protecting them with prerequisites, which feels the right thing to
> do.  The "executable bits" ones would need to become !POSIXPERM, and
> symlink ones !SYMLINKS, though.

I reworked this in the same batch as the previous git-svn patch.

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

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Thomas Gummerer]

On 01/24, Junio C Hamano wrote:
> Junio C Hamano  writes:
>
> > Sorry, but I am confused by all of the above.
> >
> > We write the thing out with write_entry(), possibly applying smudge
> > filters and eol conversion to the "git" representation to create the
> > "working tree" representation in this codepath, right?  The resulting
> > file matches what the user's configuration told us to produce.
> >
> > Until the working tree file is changed by somebody after the above
> > happens, we shouldn't have to check the contents of the file to see
> > if there is a difference.  By definition, that has to match the
> > contents expected to be there by Git.
> >
> > The only case I can think of that the above does not hold is when
> > the smuge/clean and the eol conversion are not a correct round-trip
> > operation pairs, but that would be a misconfiguration.  Otherwise,
> > we'd be _always_ comparing the contents without relying on the
> > cached stat info for any paths whose in-core and working tree
> > representations are different, not just those that are configured
> > with misbehaving smudge/clean pair, no?
>
> To put it differently, if a blob stored at path has CRLF line
> endings and .gitattributes is changed after the fact to say that it
> must have LF line endings, we should treat it as a broken transitory
> state.

Right, I wasn't considering this as a broken state, because t0025 uses
just this to transition between the states.

> There may have to be a way to "fix" an already "wrong" blob
> in the index that is milder than "rm --cached && add .", but I do
> not think write_entry(), which is shared by all the normal codepaths
> that writes out to the working tree, is the best place to do so, if
> doing so forces the contents of the paths to be always re-checked,
> just in case the user is in such a broken transitory state.

Maybe I'm misunderstanding something, but the contents of the paths
are only re-checked if we are in such a broken transition state, and
the file stored in git has crlf line endings, and thus would be
normalized.  In this case we currently re-check the contents of that
file anyway, at least when the file and the index have the same mtime,
and we actually show the correct output.

I'm not too familiar with the eol conversion code, so I might be
missing something.

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

Re: Minor bug, git ls-files -o aborts because of broken submodules

[Andreas Krey]

On Fri, 22 Jan 2016 17:26:50 +, Jeff King wrote:
...
> Here it is. I think this is the right fix, based on the previous attempt
> by Andreas and my comments. Sorry for stealing your topic,

This seems to keep happening with things I try to patch. :-)

> but I hope
> the perf numbers in the second patch will brighten your day. :)

The patches are 'quadratically' improving my case as well,
many thanks for completing this. (I was just mustering
the steam for another round of work on this.)

Andreas

-- 
"Totally trivial. Famous last words."
From: Linus Torvalds 
Date: Fri, 22 Jan 2010 07:29:21 -0800
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/1] merge-file: let conflict markers match end-of-line style of the context

[Ramsay Jones]

On 25/01/16 06:53, Johannes Schindelin wrote:
> Hi Torsten,
> 
> On Sun, 24 Jan 2016, Torsten Bögershausen wrote:
> 
>> On 24.01.16 11:48, Johannes Schindelin wrote:
>> (I had the same reasoning about the CRLF in the working tree:
>> We don't need to look at core.autocrlf/attributes, so Ack from me)
>>
>>> +test_expect_success 'conflict markers match existing line endings' '
>>> +   append_cr crlf-orig.txt &&
>>> +   append_cr crlf-diff1.txt &&
>>> +   append_cr crlf-diff2.txt &&
>>> +   test_must_fail git -c core.eol=crlf merge-file -p \
>>> +   crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
>>> +   test $(tr "\015" Q >> +   test_must_fail git -c core.eol=crlf merge-file -p \
>>> +   nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >nolf.txt &&
>>> +   test $(tr "\015" Q >> +'
>>> +
>>
>> Minor remark:
>>
>> Ramsay suggested a test that doesn't use grep or wc and looks like this:
>>
>> test_expect_success 'conflict markers contain CRLF when core.eol=crlf' '
>>   test_must_fail git -c core.eol=crlf merge-file -p \
>> nolf-diff1.txt nolf-orig.txt nolf-diff2.txt >output.txt &&
>>   tr "\015" Q |].*Q$/p" >out.txt &&
>>   cat >expect.txt <<-\EOF &&
>>   <<< nolf-diff1.txtQ
>>   ||| nolf-orig.txtQ
>>   ===Q
>>   >>> nolf-diff2.txtQ
>>   EOF
>>   test_cmp expect.txt out.txt
>> '
> 
> Probably he wrapped it at less than 192 columns per row, though ;-)
> 
;-)
> Seriously again, this longer version might test more, but it definitely
> also tests more than what I actually want to test: I am simply interested
> to verify that the conflict markers end in CR/LF when appropriate.

But you are only testing 3/4 conflict markers end in CR/LF. :-D

> Read: I
> am uncertain that I want to spend the additional lines on testing more
> than actually necessary.

If the here doc is too verbose for you, how about something like this
(totally untested):

test $(tr "\015" Q |].*Q$" | wc -l) -eq 4

instead?

HTH

ATB,
Ramsay Jones


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

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Junio C Hamano]

Johannes Schindelin  writes:

> We actually do not have to look at the *entire* context at all: if the
> files are all LF-only, or if they all have CR/LF line endings, it is
> sufficient to look at just a *single* line to match that style. And if
> the line endings are mixed anyway, it is *still* okay to imitate just a
> single line's eol: we will just add to the pile of mixed line endings,
> and there is nothing we can do about that.

Isn't there one thing we can do still?  If we use CRLF for the
marker lines when the content is already mixed, I'd think it would
help Notepad (not necessary for Notepad2 or Wordpad IIUC) by making
sure that they can see where the marker lines end correctly.

I do not care too deeply about this; just throwing it out as a
possibility to help Windowsy folks a bit more.

> Note that while it is true that there have to be at least two lines we
> can look at (otherwise there would be no conflict), the same is not true
> for line *endings*: the three files in question could all consist of a
> single line without any line ending, each. In this case we fall back to
> using LF-only.

Yeah, this is tricky, and from the same "helping Notepad that
concatenates lines with LF-only" perspective I should perhaps be
suggesting to use CRLF in such a case, too, but I would say we
should not do so.  Three variants of a LF-only file may have
conflict at the incomplete last line, and if we only look at their
"no EOL"-ness and decide to add CRLF to the result, that would be
irritatingly wrong.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Karthik Nayak]

On Mon, Jan 25, 2016 at 3:31 PM, Jeff King  wrote:
> On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > Yeah, "strip=2" would also get the job done, and extends more naturally
>> > to the branch case.
>> >
>> > To be honest, I cannot imagine anybody using anything _but_ strip=2...
>>
>> I 100% agree, and I do consider this to be internal implementation
>> detail for the listing modes of "tag" (and "branch"), which may be
>> exposed to the user (by documenting that %(refname:X) is used by
>> default), so perhaps even the flexibility of strip=2 is unwanted.
>>
>> I know what "remove-standard-prefix" is way too long for the value
>> of X above, but then we can say "the command will error out if you
>> allow your for-each-ref invocation to step outside of the area that
>> has standard prefix to be removed." without having to worry about
>> "what is the sensible thing to do when the prefixes are not what we
>> expect (too short for strip=2 or no match for short=refs/tags/)".
>
> I'm not sure "remove-standard-prefix" doesn't open its own questions.
> Like "what are the standard prefixes?".
>
> If we are going to go with "remove a prefix", I really don't think
> "remove if present" is too complicated a set of semantics (as opposed to
> "error out" you mentioned above).
>
> I do like "strip=" for its simplicity (it's easy to explain), and the
> fact that it will probably handle the git-branch case for us. The only
> open question is what to do if there are fewer components, but I really
> think any of the 4 behaviors I gave earlier would be fine.

This seems ideal, it's also quite useful like your mentioned example.
And would provide common ground for the upcoming git-branch patches
as you said.

What about a scenario wherein we have
refs/branches/abc/foo
refs/branches/xyz

should --format="%(refname:strip=3)" give us
foo
xyz (here stripping till 2 '/', which is the maximum)

or
foo
refs/branches/xyz (no stripping done at all)

I prefer the former, cause that would allow us to give a maximum value for
stripping and have everything below that maximum value stripped as well.

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

Re: GPL v2 authoritative answer on stored code as a derived work

[Junio C Hamano]

Jonathan Smith  writes:

> It's pretty clear that code stored in a Git repository isn't
> considered a derived work of Git, regardless of whether it is used
> in a commercial context or otherwise.
>
> However, I'm unable to find this stated in any authoritative and
> unambiguous manner.

Is it reasonable to ask for such a statement?  I doubt it,
especially if "It's pretty clear".

Without such a statement, I think we have already seen that the
commercial adoption is already appealing.


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

Re: [PATCH 16/19] mingw: avoid absolute path in t0008

[Ray Donnelly]

On Mon, Jan 25, 2016 at 4:48 PM, Johannes Schindelin
 wrote:
> Hi Junio,
>
> On Sun, 24 Jan 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin  writes:
>>
>> > From: Pat Thoyts 
>> >
>> > The test separator char is a colon which means any absolute paths on
>> > Windows confuse the tests that use global_excludes.
>> >
>> > Suggested-by: Karsten Blees 
>> > Signed-off-by: Pat Thoyts 
>> > Signed-off-by: Johannes Schindelin 
>> > ---
>> >  t/t0008-ignores.sh | 8 +++-
>>
>> Is the fact that $global_excludes is specified using an absolute
>> path significant to the correctness of this test script?
>
> Apparently not. So I followed your suggestion and made this independent
> of the OS:
>
> https://github.com/dscho/git/commit/0b9eb308
>
>> A larger question is if it would make more sense for Git ported to
>> Windows environment to use semicolon (that is the element separator
>> for %PATH% in the Windows land, right?) instead where POSIXy port
>> would use colon as the separator.  A variable that is a list of
>> locations (e.g. $PATH) makes little sense when elements can only be
>> relative paths in practice.
>
> Oh my... I was not looking for more work ;-)
>
> Seriously again, I do agree with the suggestion to use semicolons on
> Windows as path lists' separators instead of colons.

It definitely makes things easier from the MSYS2 path conversion
perspective. Anything that can be done to make this easier is hugely
appreciated.

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

Re: [PATCH 12/19] mingw: do not use symlinks with SVN in t9100

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > From: Karsten Blees 
> >
> > The SVN library does not seem to support symlinks, even if symlinks are
> > enabled in MSYS2 and Git. Use 'cp' instead of 'ln -s'.
> >
> > This partially fixes t/t9100-git-svn-basic.sh
> >
> > Signed-off-by: Karsten Blees 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  t/t9100-git-svn-basic.sh | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> For the purpose of SVN test, is it important that foo.link is a link
> to foo?  I am wondering what would be the fallout from making this
> change without "only on MINGW do this".

(I originally sent the following response as a reply to 13/19 by mistake.)

Your comment made me inspect the entire t9100 again, wondering why things
work when we copy the contents instead of symlinking them. And you know
what, even if I could have sworn that I verified for every patch in this
series that it is actually necessary to pass the test suite, it is *not*
necessary.

So I backed it out and it won't be part of v2 anymore.

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

Re: [RFC/PATCH 0/5] [WAS: Submodule Groups] Labels and submodule.autoInitialize

[Stefan Beller]

On Sun, Jan 24, 2016 at 11:38 AM, Jens Lehmann  wrote:
> Disclaimer: Due to my currently very limited Git time budget I only
> glanced over the recent discussion and patches. If you think I missed
> something already discussed, I'd be happy being pointed to the relevant
> discussion so I can catch up and avoid wasting everybody's time.
>
> Am 23.01.2016 um 01:31 schrieb Stefan Beller:
>>
>> This series introduces labels which you can attach to submodules like so:
>>
>>  $ cat .gitmodules
>>  [submodule "gcc"]
>>  path = gcc
>>  url = git://...
>>  label = default
>>  label = devel
>>  [submodule "linux"]
>>  path = linux
>>  url = git://...
>>  label = default
>>
>>  $ git submodule add --name emacs --label "editor" --label default
>> git://...
>>
>>  # If upstream has submodules properly labeled, you can make use of
>> them:
>
>
> Cool. Without having looked at the code I assume you also can label
> submodules yourself in .git/config (or your global config) to override
> upstream's settings (or use your own labels if .gitmodules does not
> contain any)?

I am not sure. I'll add a test for that in a reroll and make sure it passes.

>
>>  $ git config --add submodule.autoInitialize "*default"
>>  $ git config --add submodule.autoInitialize ":name"
>>  $ git config --add submodule.autoInitialize "./by/path"
>
>
> Ok. Though we might wanna call it submodule.autoUpdate, as initializing
> it is only the prerequisite for automatically updating submodules. And
> I believe automatically updating is the thing we're after here, right?

I am not sure here, too. I would not mind an occasional "git submodule update"
for whenever I want upstream to come down on my disk. However that's what I
do with "git pull" in the non-submodule case, so you'd expect git pull to
also run the update strategies for all submodules which are configured to
autoUpdate?

That makes sense to me. Though I never use "git pull" to begin with.
I always use fetch and see how to go from there (merge or rebase
after inspecting the code I fetched). That would mean we want to
add the autoUpdate strategy to merge/rebase and the fetching of
submodules to the fetch command?

>
> I'll try to explain why I believe we should be generous in initializing
> submodules: If a submodule in one branch has a label configured to be
> automatically updated and hasn't got the same label in another branch,
> we still need to initialize the submodule even when we are on the latter
> branch in case the user switches to the first branch, right?

No. "git checkout" ought to autoInitalize the submodule in question when
switching branches. I don't want to see initialized, but unused submodules
around (neither empty dirs nor in the .git/config ideally)?


> And the
> fetch command needs to fetch submodule changes too when they happen in
> a branch where this submodule is part of a label group configured to be
> updated automatically, no matter what is currently found in the work
> tree.

Right, as said above fetch needs to fetch all the submodules as well. I wonder
if it needs to fetch all submodule sha1s only or just try to get as
much from the
submodule as possible.

>
> So I'd propose to:
>
> *) Initialize every submodule present on clone or newly fetched when
>the autoUpdate config is set.

What if you clone branch A and then switch to B ? B has a submodule which
was not initialized in A. I do not think initializing on clone/fetch
is the right thing
to do, but rather the branch switching command (checkout) shall make sure
all its autoUpdate/autoInitialze submodules are setup properly, no?

>
> *) Automatically fetch only those submodules that are changed in
>a commit where they have a label configured (in the commit's
>.gitmodules or locally) that is to be checked out.

Not sure I follow here.

>
> *) Let "git submodule update" update only those submodules that
>have an autoupdate label configured.

Why not update all initialized submodules? (In my imagination
"all initialized submodules" are equal to "all submodules the user is
interested in", i.e. when going from branch A to B, the checkout will
(de-)init submodules as necessary.

>
> That will make switching between branches with different label
> configurations work fine. Or am I missing something here?
>
> And we need to teach diff and status to complain about empty work
> trees and missing initialization of submodules that are to be
> automatically updated too.

What about empty work trees?

I'll add "git status" complaining about missing initialized submodules.

>
>>  # The prefix * denotes a label as found in .gitmodules
>>  # : goes before names
>>  # path are prefixed ./ currently
>>  # both path and names need work
>
>
> Question: how do I configure all submodules to be automatically
> initialized without having to give them a label? "./*"? Or just
> setting the option 

Re: [PATCH v7 01/11] dir: free untracked cache when removing it

[Stefan Beller]

On Sun, Jan 24, 2016 at 7:28 AM, Christian Couder
 wrote:
> Signed-off-by: Christian Couder 
> ---
>  builtin/update-index.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index 7431938..a6fff87 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -1123,6 +1123,7 @@ int cmd_update_index(int argc, const char **argv, const 
> char *prefix)
> add_untracked_ident(the_index.untracked);
> the_index.cache_changed |= UNTRACKED_CHANGED;
> } else if (!untracked_cache && the_index.untracked) {
> +   free_untracked_cache(the_index.untracked);

Do we need to free its members, too? (Or is it empty enough here,
that there are no memleaks in there? If this were the case a hint in
the commit message would be helpful)

> the_index.untracked = NULL;
> the_index.cache_changed |= UNTRACKED_CHANGED;
> }
> --
> 2.7.0.181.gd7ef666.dirty
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/19] mingw: accomodate t0060-path-utils for MSYS2

[Johannes Schindelin]

Hi Hannes,

On Sun, 24 Jan 2016, Johannes Sixt wrote:

> Am 24.01.2016 um 16:44 schrieb Johannes Schindelin:
> > On Windows, there are no POSIX paths, only Windows ones (an absolute
> > Windows path looks like "C:\Program Files\Git\ReleaseNotes.html", under
> > most circumstances, forward slashes are also allowed and synonymous to
> > backslashes).
> > 
> > So when a POSIX shell (such as MSYS2's Bash, which is used by Git for
> > Windows to execute all those shell scripts that are part of Git) passes
> > a POSIX path to test-path-utils.exe (which is not POSIX-aware), the path
> > is translated into a Windows path. For example, /etc/profile becomes
> > C:/Program Files/Git/etc/profile.
> > 
> > This path translation poses a problem when passing the root directory as
> > parameter to test-path-utils.exe, as it is not well defined whether the
> > translated root directory should end in a slash or not. MSys1 stripped
> > the trailing slash, but MSYS2 does not.
> > 
> > To work with both behaviors, we simply test what the current system does
> > in the beginning of t0060-path-utils.sh and then adjust the expected
> > longest ancestor length accordingly.
> > 
> > Originally, the Git for Windows project patched MSYS2's runtime to
> > accomodate Git's regression test, but we really should do it the other
> > way round.
> > 
> > Thanks to Ray Donnelly for his patient help with this issue.
> > 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >   t/t0060-path-utils.sh | 37 ++---
> >   1 file changed, 22 insertions(+), 15 deletions(-)
> > 
> > diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> > index f0152a7..89d03e7 100755
> > --- a/t/t0060-path-utils.sh
> > +++ b/t/t0060-path-utils.sh
> > @@ -7,6 +7,13 @@ test_description='Test various path utilities'
> >   
> >   . ./test-lib.sh
> >   
> > +# On Windows, the root directory "/" is translated into a Windows 
> > directory.
> > +# As it is not well-defined whether that Windows directory should end in a
> > +# slash or not, let's test for that and adjust our expected longest 
> > ancestor
> > +# length accordingly.
> > +root_offset=0
> > +case "$(test-path-utils print_path /)" in ?*/) root_offset=-1;; esac
> > +
> >   norm_path() {
> > expected=$(test-path-utils print_path "$2")
> > test_expect_success $3 "normalize path: $1 => $2" \
> 
> In t0060-path-utils.sh, I currently find this:
> 
> # On Windows, we are using MSYS's bash, which mangles the paths.
> # Absolute paths are anchored at the MSYS installation directory,
> # which means that the path / accounts for this many characters:
> rootoff=$(test-path-utils normalize_path_copy / | wc -c)
> # Account for the trailing LF:
> if test $rootoff = 2; then
>   rootoff=# we are on Unix
> else
>   rootoff=$(($rootoff-1))
> fi
> 
> ancestor() {
>   # We do some math with the expected ancestor length.
>   expected=$3
>   if test -n "$rootoff" && test "x$expected" != x-1; then
>   expected=$(($expected+$rootoff))
>   fi
>   test_expect_success "longest ancestor: $1 $2 => $expected" \
>   "actual=\$(test-path-utils longest_ancestor_length '$1' '$2') &&
>test \"\$actual\" = '$expected'"
> }
> 
> Furthermore, since you are modifying only cases where the expected
> value is not -1 and we already have a check for this case in the
> helper function, wouldn't it be simpler to merge the offset that your
> case needs with the one that we already have?

Good points. I reworked the patch here (will be part of v2):
https://github.com/dscho/git/commit/24767bd

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

Re: [PATCH 16/19] mingw: avoid absolute path in t0008

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > From: Pat Thoyts 
> >
> > The test separator char is a colon which means any absolute paths on
> > Windows confuse the tests that use global_excludes.
> >
> > Suggested-by: Karsten Blees 
> > Signed-off-by: Pat Thoyts 
> > Signed-off-by: Johannes Schindelin 
> > ---
> >  t/t0008-ignores.sh | 8 +++-
> 
> Is the fact that $global_excludes is specified using an absolute
> path significant to the correctness of this test script?

Apparently not. So I followed your suggestion and made this independent
of the OS:

https://github.com/dscho/git/commit/0b9eb308

> A larger question is if it would make more sense for Git ported to
> Windows environment to use semicolon (that is the element separator
> for %PATH% in the Windows land, right?) instead where POSIXy port
> would use colon as the separator.  A variable that is a list of
> locations (e.g. $PATH) makes little sense when elements can only be
> relative paths in practice.

Oh my... I was not looking for more work ;-)

Seriously again, I do agree with the suggestion to use semicolons on
Windows as path lists' separators instead of colons.

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

Re: [PATCH 01/19] mingw: let's use gettext with MSYS2

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This solves two problems:
> >
> > - we now have proper localisation even on Windows
> >
> > - we sidestep the infamous "BUG: your vsnprintf is broken (returned -1)"
> >   message when running "git init" (which otherwise prevents the entire
> >   test suite from running)
> 
> It is unclear to me how gettext is related to use of vsnprintf().

Including libintl.h overrides vsnprintf() with the libintl_vsnprintf()
drop-in replacement that does return the expected value.

I updated the commit message (will be part of v2):

https://github.com/dscho/git/commit/4473801e

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

Re: [PATCH 13/19] mingw: outsmart MSYS2's path substitution in t1508

[Johannes Schindelin]

Hi Junio,

On Mon, 25 Jan 2016, Johannes Schindelin wrote:

> On Sun, 24 Jan 2016, Junio C Hamano wrote:
> 
> > Johannes Schindelin  writes:
> > 
> > > From: Thomas Braun 
> > >
> > > A string of the form "@/abcd" is considered a file path
> > > by the msys layer and therefore translated to a Windows path.
> > >
> > > Here the trick is to double the slashes.
> > >
> > > The MSYS2 patch translation can be studied by calling
> > >
> > >   test-path-utils print_path 
> > >
> > > Signed-off-by: Thomas Braun 
> > > Signed-off-by: Johannes Schindelin 
> > > ---
> > 
> > This feels wrong.
> > 
> > The point of this test is that you can ask to checkout a branch
> > whose name is a strangely looking "@/at-test", and a ref whose name
> > is "refs/heads/@/at-test" indeed is created.
> > 
> > The current "checkout" may be lazy and not signal an error for a
> > branch name with two consecutive slashes, but I wouldn't be
> > surprised if we tighten that later, and more importantly, I do not
> > think we ever promised users if you asked a branch "a//b" to be
> > created, we would create "refs/heads/a/b".
> > 
> > The new test hardcodes and promises such an incompatible behaviour,
> > i.e. a request to create "@//b" results in "@/b" created, only to
> > users on MINGW, fracturing the expectations of the Git userbase.
> > 
> > Wouldn't it be better to declare "On other people's Git, @/foo is
> > just as normal a branch name as a/foo, but on MINGW @/foo cannot be
> > used" by skipping some tests using prerequisites instead?
> 
> As Eric points out, this is not so much a behavior on Git as of the MSYS2
> Bash. In fact, if you call `git.exe checkout -b @/at-test` from a cmd
> window, it works just as advertised.
> 
> But your comment made me inspect the entire t9100 again, wondering why
> things work when we copy the contents instead of symlinking them. And you
> know what, even if I could have sworn that I verified for every patch in
> this series that it is actually necessary to pass the test suite, it is
> *not* necessary.
> 
> So I backed it out and it won't be part of v2 anymore.

Whoops. This was meant to be a comment on your comment on 12/19. I'll
reply to the appropriate mail...

As to the patch 13/19 that we are discussing here, I agree that it is
better to simply skip the test with the offending argument. See

https://github.com/dscho/git/commit/ca5edbe

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

Re: [PATCH 00/19] Let Git's tests pass on Windows

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > This is a big milestone. With these modifications, Git's source code
> > does not only build without warnings in Git for Windows' SDK, but
> > passes the entire regression test suite.
> 
> ;-)
> 
> It is somewhat surprising that with only these changes my tree is no
> longer behind the Git for Windows effort--all tests pass so everything
> should be working perfectly, right ;-)

Oh, don't you worry, there are plenty more patches in Git for Windows'
fork. Some of them will become irrelevant, others probably already did so.
The majority, though, I think will be of interest and I will submit them
over the next weeks/months (it will take a while not only because I do not
want to overload the list, but also because I have to do quite a bit of
spring cleaning).

> There were a few changes in the series that raised my eyebrows,
> which I'll respond separately, but overall it's a great
> achievement.

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

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Thomas Gummerer  writes:

> On 01/24, Junio C Hamano wrote:
>> To put it differently, if a blob stored at path has CRLF line
>> endings and .gitattributes is changed after the fact to say that it
>> must have LF line endings, we should treat it as a broken transitory
>> state.
>
> Right, I wasn't considering this as a broken state, because t0025 uses
> just this to transition between the states.
>
>> There may have to be a way to "fix" an already "wrong" blob
>> in the index that is milder than "rm --cached && add .", but I do
>> not think write_entry(), which is shared by all the normal codepaths
>> that writes out to the working tree, is the best place to do so, if
>> doing so forces the contents of the paths to be always re-checked,
>> just in case the user is in such a broken transitory state.
>
> Maybe I'm misunderstanding something, but the contents of the paths
> are only re-checked if we are in such a broken transition state, and

What I do not understand here is how the added check ensures that
"only if in such a broken transition state".  would_convert_to_git()
does not take the contents but is called only with the pathname to
key into the attributes, so in a typical cross platform project
where all the source files are "text" and the repository can set
core.eol to adjust the end of line convention for its working tree,
the added check has no way to differentiate the paths that are
recorded with CRLF line endings in the object database by mistake,
i.e. the ones in the broken transitory state, and all the other
paths that are following the "text" and storing their blobs with LF
line endings.  The added check would trigger "is it racy" check to
re-reads the contents that we have written out from the working tree
for the paths with "wrong" blobs, but how would it avoid doing so
for the paths whose blobs are already stored correctly?

The new code affects not just "reset --hard", but everybody who
writes out from the object database to the working tree and records
that these paths are checked out in the index.  How does the new
code avoid destroying the performance for all paths?

> the file stored in git has crlf line endings, and thus would be
> normalized.  In this case we currently re-check the contents of that
> file anyway, at least when the file and the index have the same mtime,
> and we actually show the correct output.

The right way to at that "correct output", I think, is that it
happens to be shown that way by accident.  It is questionable that
it is correct to report that such a path is modified.  Immediately
after you check out a path to the working tree out of the index, via
"reset --hard" and "checkout $path", by definition it ought to be
the same between the working tree file and the index.

Unless it is in this transititory broken state, that is.

The "by accident" happens only because racy-git avoidance is being
implemented in one particular way.  Is about protecting people from
making changes to the working tree files immediately after their
previous contents are registered to the index (and the index files
written to the disk), and immediately after we write things out of
the index and by definition the result and the indexed blob ought to
match there is no reason to re-read and recheck unless the working
tree files are further edited.

The current way the racy-git avoidance works by re-reading and
re-checking the contents when the timestamps match is merely one
possible implementation, and that is the only thing that produces
your "correct" output most of the time in this test.  We could have
waited after writing the index time for a second before giving the
control back to the user instead, which would have also allowed us
to implement the racy-git avoidance, and in that alternate world,
all these transitory broken paths would have been correctly reported
as unmodified.

IOW, I would think the test in question is insisting a single
outcome for an operation whose result is undefined, and it is
harmful to twist the system by pessimizing the common cases just
to cater to this transititory broken state.

I am not saying that we shouldn't have support for users to fix
their repository and get out of this transititory broken state.  A
recent work by Torsten Bögershausen to have ls-files report the end
of line convention used in the blob in the index and the settings
that affect conversion for each path (among other things) is a step
in the right direction.  With a support like that, those who noticed
that they by mistake added CRLF files to the index as-is when they
wanted their project to be cross platform can recover from it by
setting necessary attributes (i.e. mark them as "text") and then
find paths that are broken out of "ls-files --eol" output to see
which ones are not using lf end-of-line in the index.

I do not think there is a canned command to help dealing with these
broken paths right now.  You would have to check them out of the
index (you would 

Re: [PATCH 13/19] mingw: outsmart MSYS2's path substitution in t1508

[Johannes Schindelin]

Hi Junio,

On Sun, 24 Jan 2016, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > From: Thomas Braun 
> >
> > A string of the form "@/abcd" is considered a file path
> > by the msys layer and therefore translated to a Windows path.
> >
> > Here the trick is to double the slashes.
> >
> > The MSYS2 patch translation can be studied by calling
> >
> > test-path-utils print_path 
> >
> > Signed-off-by: Thomas Braun 
> > Signed-off-by: Johannes Schindelin 
> > ---
> 
> This feels wrong.
> 
> The point of this test is that you can ask to checkout a branch
> whose name is a strangely looking "@/at-test", and a ref whose name
> is "refs/heads/@/at-test" indeed is created.
> 
> The current "checkout" may be lazy and not signal an error for a
> branch name with two consecutive slashes, but I wouldn't be
> surprised if we tighten that later, and more importantly, I do not
> think we ever promised users if you asked a branch "a//b" to be
> created, we would create "refs/heads/a/b".
> 
> The new test hardcodes and promises such an incompatible behaviour,
> i.e. a request to create "@//b" results in "@/b" created, only to
> users on MINGW, fracturing the expectations of the Git userbase.
> 
> Wouldn't it be better to declare "On other people's Git, @/foo is
> just as normal a branch name as a/foo, but on MINGW @/foo cannot be
> used" by skipping some tests using prerequisites instead?

As Eric points out, this is not so much a behavior on Git as of the MSYS2
Bash. In fact, if you call `git.exe checkout -b @/at-test` from a cmd
window, it works just as advertised.

But your comment made me inspect the entire t9100 again, wondering why
things work when we copy the contents instead of symlinking them. And you
know what, even if I could have sworn that I verified for every patch in
this series that it is actually necessary to pass the test suite, it is
*not* necessary.

So I backed it out and it won't be part of v2 anymore.

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

Re: [PATCH 13/19] mingw: outsmart MSYS2's path substitution in t1508

[Junio C Hamano]

Eric Sunshine  writes:

> On Sun, Jan 24, 2016 at 9:03 PM, Junio C Hamano  wrote:
>> The new test hardcodes and promises such an incompatible behaviour,
>> i.e. a request to create "@//b" results in "@/b" created, only to
>> users on MINGW, fracturing the expectations of the Git userbase.
>
> What the commit message doesn't explain is that ...
> ...
> This commit message is trying to say that MSYS shell undesirably sees
> @/fish as an absolute path, thus tries translating it to a Windows
> path, such as @C:\fish. The only way to suppress this unwanted
> translation is to manually double the slash, hence the patch makes the
> test use @//fish which, when finally seen by the program, is just
> @/fish, as was intended in the first place. So, doubling the slash on
> MINGW is not promising incompatible behavior for MINGW users; it's
> just working around unwanted path translation of the shell.

Ah, OK, thanks for clarifying it.  Presumably you would then use
"checkout @//b" to switch to it, and "log @//b" to look at its
hsitory.  When you read "git branch" output and see "@/b" in it, you
would also not complain thinking "oh I thought I created "@//b", not
with a single branch!".

Then no issues on allowing "checkout -b @//b" to create a branch
"@/b" from me.

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

Re: [PATCH 01/19] mingw: let's use gettext with MSYS2

[Junio C Hamano]

Johannes Schindelin  writes:

>> > - we sidestep the infamous "BUG: your vsnprintf is broken (returned -1)"
>> >   message when running "git init" (which otherwise prevents the entire
>> >   test suite from running)
>> 
>> It is unclear to me how gettext is related to use of vsnprintf().
>
> Including libintl.h overrides vsnprintf() with the libintl_vsnprintf()
> drop-in replacement that does return the expected value.

Ah, that makes sense.

>
> I updated the commit message (will be part of v2):
>
>   https://github.com/dscho/git/commit/4473801e

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

Re: [PATCH 3/5] submodule-config: keep labels around

[Stefan Beller]

On Sun, Jan 24, 2016 at 10:06 AM, Sebastian Schuberth
 wrote:
> On Sat, Jan 23, 2016 at 1:31 AM, Stefan Beller  wrote:
>
>> We need the submodule groups in a later patch.
>
> The commit message should now say "labels", too, I guess.

Sure, thanks for catching!

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

Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak  wrote:
> From align_atom_parser() form parse_align_position() which given a
> string would give us the alignment position. This is a preparatory
> patch as to introduce prefixes for the %(align) atom and avoid
> redundancy in the code.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
> +static align_type parse_align_position(const char *s)
> +{
> +   if (!strcmp(s, "right"))
> +   return ALIGN_RIGHT;
> +   else if (!strcmp(s, "middle"))
> +   return ALIGN_MIDDLE;
> +   else if (!strcmp(s, "left"))
> +   return ALIGN_LEFT;
> +   return -1;
> +}

This code was just moved in patch 9/15 and is being relocated again
here in patch 10/15. If you change the order of the patches so that
this preparatory refactoring is done first, the diff of the "introduce
align_atom_parser()" patch will become smaller and be a bit easier to
review. (Plus it just makes sense to do preparation first.)

>  static void align_atom_parser(struct used_atom *atom)
>  {
> struct align *align = >u.align;
> @@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
> align->position = ALIGN_LEFT;
>
> while (*s) {
> +   int position;
> buf = s[0]->buf;
>
> if (!strtoul_ui(buf, 10, (unsigned int *)))
> ;
> -   else if (!strcmp(buf, "left"))
> -   align->position = ALIGN_LEFT;
> -   else if (!strcmp(buf, "right"))
> -   align->position = ALIGN_RIGHT;
> -   else if (!strcmp(buf, "middle"))
> -   align->position = ALIGN_MIDDLE;
> +   else if ((position = parse_align_position(buf)) >= 0)
> +   align->position = position;
> else
> die(_("unrecognized %%(align) argument: %s"), buf);
> s++;
> --
> 2.6.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 09/11] config: add core.untrackedCache

[Junio C Hamano]

Christian Couder  writes:

> diff --git a/read-cache.c b/read-cache.c
> index 5be7cd1..a04ec8c 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1497,10 +1497,23 @@ static struct cache_entry *create_from_disk(struct 
> ondisk_cache_entry *ondisk,
>   return ce;
>  }
>  
> -static void check_ce_order(struct index_state *istate)
> +static void post_read_index_from(struct index_state *istate)
>  {
>   unsigned int i;
>  
> + switch (git_config_get_untracked_cache()) {
> + case -1: /* keep: do nothing */
> + break;
> + case 0: /* false */
> + remove_untracked_cache(istate);
> + break;
> + case 1: /* true */
> + add_untracked_cache(istate);
> + break;
> + default: /* unknown value: do nothing */
> + break;
> + }
> +
>   for (i = 1; i < istate->cache_nr; i++) {
>   struct cache_entry *ce = istate->cache[i - 1];
>   struct cache_entry *next_ce = istate->cache[i];

Bad manners.

 * The new code added to an existing function, unless there is a
   good reason, goes to the bottom.  In this case, the verification
   of the ordering of cache entries and tweaking of UC extension are
   two unrelated things that can be independently done, and there is
   no justification why the new code has to come to top.

 * The old function name served as a good documentation of what it
   does.  That is no longer the case.  Each unrelated segment of
   this new function needs to be commented.  Even better, perhaps
   leave the original check_ce_order() as-is, introduce a new
   function tweak_uc_extension(), and make the post_read_index()
   to be just two-liner function:

static void post_read_index(struct index_state *istate)
{
check_ce_order(istate);
tweak_uc_extension(istate);
}

   That way the documentation value of each function that does one
   specific thing and named specific to its task will be kept, and
   there is no need for extra comments.


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

[PATCH] mingw: avoid linking to the C library's isalpha()

[Johannes Sixt]

The implementation of mingw_skip_dos_drive_prefix() calls isalpha() via
has_dos_drive_prefix(). Since the definition occurs long before isalpha()
is defined in git-compat-util.h, my build environment reports:

CC alloc.o
In file included from git-compat-util.h:186,
 from cache.h:4,
 from alloc.c:12:
compat/mingw.h: In function 'mingw_skip_dos_drive_prefix':
compat/mingw.h:365: warning: implicit declaration of function 'isalpha'

Dscho does not see a similar warning in his build and suspects that
ctype.h is included somehow behind the scenes. This implies that his build
links to the C library's isalpha() and does not use git's isalpha().

To fix both the warning in my build and the inconsistency in Dscho's
build, move the function definition to mingw.c. Then it picks up git's
isalpha() because git-compat-util.h is included at the top of the file.

Signed-off-by: Johannes Sixt 
---
 compat/mingw.c | 7 +++
 compat/mingw.h | 7 +--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+   int ret = has_dos_drive_prefix(*path);
+   *path += ret;
+   return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-   int ret = has_dos_drive_prefix(*path);
-   *path += ret;
-   return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
-- 
2.7.0.118.g90056ae

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

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Junio C Hamano]

Junio C Hamano  writes:

> I am not saying that we shouldn't have support for users to fix
> their repository and get out of this transititory broken state.  A
> recent work by Torsten Bögershausen to have ls-files report the end
> of line convention used in the blob in the index and the settings
> that affect conversion for each path (among other things) is a step
> in the right direction.  With a support like that, those who noticed
> that they by mistake added CRLF files to the index as-is when they
> wanted their project to be cross platform can recover from it by
> setting necessary attributes (i.e. mark them as "text") and then
> find paths that are broken out of "ls-files --eol" output to see
> which ones are not using lf end-of-line in the index.
>
> I do not think there is a canned command to help dealing with these
> broken paths right now.  You would have to check them out of the
> index (you would get a CRLF file in the working tree in the example
> we are discussing), fix the line endings (you would run dos2unix on
> it in this example, as you would want "text=true" attribute) and
> "git add" them to recover manually, but I can imagine that Torsten's
> work can be extended to do all of these, without molesting the
> working tree files, with minimum work by the end user.  That is:
>
>  * Reuse Torsten's "ls-files --eol" code to find paths that record
>the blob in the index that does not follow the eol convention
>specified for the path;
>
>  * For each of these index entries, run convert_to_working_tree() on
>the indexed contents, and then on the result of it, run
>convert_to_git().  The result is the blob that the index ought to
>have had, if it were to be consistent with the attribute
>settings.  So add that to the index.
>
>  * Write the index out.
>
>  * Tell the user to commit or commit it automatically with a canned
>log message "fix broken encoding" or something.

Here is what I whipped up as a lunch-break hack.  I do not claim
that "git add" would be the best place to do this, but it should be
sufficient to illustrate the overall idea.

The user can say "git add --fix-index" and have a simplified version
of the above happen, i.e. for each path in the index, if the
contents recorded there does not round-trip to the identical
contents when first converted to the working tree representation
(i.e. passing through core.eol and smudge filter conversion) and
then converted back to the Git blob representation (i.e. clean
filter and core.crlf), and when the result is different from what we
started from, we know we have an unnormalized blob registered in the
index, so we replace it.  After this, "git diff --cached" would show
the correction made by this operation, and committing it would let
you fix the earlier mistake that added CRLF content when the path
was marked with text=true attribute.

We could go even fancier and attempt the round-trip twice or more.
It is possible that the in-index representation will not converge
when you use a misconfigured pair of clean/smudge filters (e.g.
using "gzip -d -c" as the smudge filter, and then using "gzip -c"
without "-n" option as the clean filter would most likely make the
in-index representation fuzzy, as each time the cycle is run, the
compressed contents will be made with different timestamps, even
though the working tree representation will be the same), and an
operation "we screwed up the filters, please repair the damage!"
like this "add --fix-index" is probably the best place to catch such
a misconfiguration.

 builtin/add.c | 64 +++
 1 file changed, 64 insertions(+)

diff --git a/builtin/add.c b/builtin/add.c
index 145f06e..36d3915 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -233,6 +233,7 @@ N_("The following paths are ignored by one of your 
.gitignore files:\n");
 
 static int verbose, show_only, ignored_too, refresh_only;
 static int ignore_add_errors, intent_to_add, ignore_missing;
+static int fix_index;
 
 #define ADDREMOVE_DEFAULT 1
 static int addremove = ADDREMOVE_DEFAULT;
@@ -263,6 +264,7 @@ static struct option builtin_add_options[] = {
OPT_BOOL( 0 , "refresh", _only, N_("don't add, only refresh the 
index")),
OPT_BOOL( 0 , "ignore-errors", _add_errors, N_("just skip files 
which cannot be added because of errors")),
OPT_BOOL( 0 , "ignore-missing", _missing, N_("check if - even 
missing - files are ignored in dry run")),
+   OPT_BOOL( 0 , "fix-index", _index, N_("fix contents in the index 
that is inconsistent with the eol and clean/smudge filters")),
OPT_END(),
 };
 
@@ -297,6 +299,64 @@ static int add_files(struct dir_struct *dir, int flags)
return exit_status;
 }
 
+static int fix_index_roundtrip(int ac, const char **av, const char *prefix)
+{
+   int i;
+
+   if (ac)
+   die(_("git add --fix-index does not take any other argument"));
+
+   if (read_cache() 

[RFC] tag-ref and tag object binding

[Santiago Torres]

Hello everyone.

I've done some further research on the security properties of git
metadata and I think I've identified something that might be worth
discussing. In this case, The issue is related to the refs that point to
git tag objects. Specifically, the "loose" nature of tag refs might
possibly trick people into installing the wrong revision (version?) of a
file.

To elaborate, the ref of a tag object can be moved around in the same
way a branch can be moved around (either manually or by using git
commands). If someone with write access can modify where this ref points
to, and points it to another valid tag (e.g., an older, vulnerable
version), then many tools that work under the assumption of static tags
might mistakenly install/pull the wrong reivision of source code. I've
verified that this is possible to pull off in package managers such as
PIP, rubygems, gradle(maven), as well as git submodules tracking tags.

In order to stay loyal to the way files in the .git directory are
ordered, I don't think that making the ref file (or packed refs) files
differently is an option. However, I think that it could be possible to
store the "origin ref" in the git tag object, so tools can verify that
they are looking at the appropriate tag. There might also be a simpler
solution to this, and I would appreciate any feedback.

What do you guys think?

Thanks!
Santiago.


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

Re: [PATCH] mingw: avoid linking to the C library's isalpha()

[Junio C Hamano]

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

Re: [PATCH 16/19] mingw: avoid absolute path in t0008

[Junio C Hamano]

Johannes Schindelin  writes:

>> Is the fact that $global_excludes is specified using an absolute
>> path significant to the correctness of this test script?
>
> Apparently not. So I followed your suggestion and made this independent
> of the OS:
>
>   https://github.com/dscho/git/commit/0b9eb308

I see many "cd" in the script, some of which come before the global
exclude file is enabled, but others ("--stdin from subdirectory",
for example) do run from a subdirectory.  It turns out that this
file is used as the value of core.excludesfile.  By the time that
file is read in setup_standard_excludes(), presumably we have
already done setup_git_directory() and moved back to the root of the
working tree, so I guess such a change would be harmless.

It somehow makes me feel dirty, though.  But that is not a specific
problem to mingw port.


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

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak  wrote:
> Introduce optional prefixes "width=" and "position=" for the align atom
> so that the atom can be used as "%(align:width=,position=)".
>
> Add Documetation and tests for the same.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index fe4796c..0c4417f 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -133,6 +133,47 @@ test_expect_success 'right alignment' '
> +cat >expect <<-\EOF
> +|   refname is refs/heads/master   |refs/heads/master
> +|refname is refs/heads/side|refs/heads/side
> +| refname is refs/odd/spot |refs/odd/spot
> +| refname is refs/tags/double-tag  |refs/tags/double-tag
> +|refname is refs/tags/four |refs/tags/four
> +| refname is refs/tags/one |refs/tags/one
> +| refname is refs/tags/signed-tag  |refs/tags/signed-tag
> +|refname is refs/tags/three|refs/tags/three
> +| refname is refs/tags/two |refs/tags/two
> +EOF
> +
> +test_align_permutations() {

Style: in shell scripts, add space before ()

> +   while read -r option; do

Style: drop semi-colon and place 'do' on its own line

> +   test_expect_success 'align permutations' '

This title is not as illuminating as it could be. It would be better
to indicate which permutation is being tested. For instance:

test_expect_success "align:$option" ...

Note the double-quotes. Or:

test_expect_success "align permutation: $option" ...

or something.

> +   git for-each-ref --format="|%(align:$option)refname is 
> %(refname)%(end)|%(refname)" >actual &&

This is not wrong, per se, but referencing $option inside the
non-interpolating single-quote context of the test body makes it a bit
harder to understand than it need be. As it is, at the time that the
test body actually gets executed, $option still evaluates to the
desired permutation value so it works. However, it would show intent
more clearly and be more robust to use a double-quote context to
interpolate $option into the git-for-each-ref invocation directly
rather than allowing the test body to pick up the value at execution
time.

Fixing this means using double- rather than single-quotes for the test
body, which means you'd also want to flip the  double-quotes wrapping
the --format= argument over to single-quotes. Also, for style
consistency, indent the test body. The end result should be something
like this:

test_align_permutations () {
while ...
do
test_expect_success "align:$option" "
git for-each-ref --format='...$option...' >actual &&
...
"
done
}

> +   test_cmp expect actual
> +   '
> +   done;

Style: drop the semi-colon

More below...

> +}
> +
> +test_align_permutations <<-\EOF
> +   middle,42
> +   42,middle
> +   position=middle,42
> +   42,position=middle
> +   middle,width=42
> +   width=42,middle
> +   position=middle,width=42
> +   width=42,position=middle
> +EOF
> +
> +# Last one wins (silently) when multiple arguments of the same type are given
> +
> +test_align_permutations <<-\EOF
> +   32,width=42,middle
> +   width=30,42,middle
> +   width=42,position=right,middle
> +   42,right,position=middle
> +EOF

Overall, this version is much nicer now that the tests are
table-driven rather than each permutation being copied/pasted.

This is a tangent, but it's disappointing that this entire test script
is skipped if GPG is not installed, especially since none of these
tests seem to care at all about signed tags. By requiring GPG
(unnecessarily), these tests likely are not getting run as widely as
they ought to. Consequently, it probably would be a good idea to drop
the GPG requirement from the top of the file and have the "setup" test
create lightweight tags ("git tag ...") rather than signed ones ("git
tag -s ...").
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Thomas Gummerer]

On 01/25, Junio C Hamano wrote:
> Thomas Gummerer  writes:
>
> > On 01/24, Junio C Hamano wrote:
> >> To put it differently, if a blob stored at path has CRLF line
> >> endings and .gitattributes is changed after the fact to say that it
> >> must have LF line endings, we should treat it as a broken transitory
> >> state.
> >
> > Right, I wasn't considering this as a broken state, because t0025 uses
> > just this to transition between the states.
> >
> >> There may have to be a way to "fix" an already "wrong" blob
> >> in the index that is milder than "rm --cached && add .", but I do
> >> not think write_entry(), which is shared by all the normal codepaths
> >> that writes out to the working tree, is the best place to do so, if
> >> doing so forces the contents of the paths to be always re-checked,
> >> just in case the user is in such a broken transitory state.
> >
> > Maybe I'm misunderstanding something, but the contents of the paths
> > are only re-checked if we are in such a broken transition state, and
>
> What I do not understand here is how the added check ensures that
> "only if in such a broken transition state".  would_convert_to_git()
> does not take the contents but is called only with the pathname to
> key into the attributes, so in a typical cross platform project
> where all the source files are "text" and the repository can set
> core.eol to adjust the end of line convention for its working tree,
> the added check has no way to differentiate the paths that are
> recorded with CRLF line endings in the object database by mistake,
> i.e. the ones in the broken transitory state, and all the other
> paths that are following the "text" and storing their blobs with LF
> line endings.  The added check would trigger "is it racy" check to
> re-reads the contents that we have written out from the working tree
> for the paths with "wrong" blobs, but how would it avoid doing so
> for the paths whose blobs are already stored correctly?
>
> The new code affects not just "reset --hard", but everybody who
> writes out from the object database to the working tree and records
> that these paths are checked out in the index.  How does the new
> code avoid destroying the performance for all paths?

I misunderstood the way would_convert_to_git() works, I should have
actually read the code, instead of just relying on my test, which was
even wrong.  Sorry about the confusion, my patch does indeed hurt
the performance.

> > the file stored in git has crlf line endings, and thus would be
> > normalized.  In this case we currently re-check the contents of that
> > file anyway, at least when the file and the index have the same mtime,
> > and we actually show the correct output.
>
> The right way to at that "correct output", I think, is that it
> happens to be shown that way by accident.  It is questionable that
> it is correct to report that such a path is modified.  Immediately
> after you check out a path to the working tree out of the index, via
> "reset --hard" and "checkout $path", by definition it ought to be
> the same between the working tree file and the index.
>
> Unless it is in this transititory broken state, that is.
>
> The "by accident" happens only because racy-git avoidance is being
> implemented in one particular way.  Is about protecting people from
> making changes to the working tree files immediately after their
> previous contents are registered to the index (and the index files
> written to the disk), and immediately after we write things out of
> the index and by definition the result and the indexed blob ought to
> match there is no reason to re-read and recheck unless the working
> tree files are further edited.
>
> The current way the racy-git avoidance works by re-reading and
> re-checking the contents when the timestamps match is merely one
> possible implementation, and that is the only thing that produces
> your "correct" output most of the time in this test.  We could have
> waited after writing the index time for a second before giving the
> control back to the user instead, which would have also allowed us
> to implement the racy-git avoidance, and in that alternate world,
> all these transitory broken paths would have been correctly reported
> as unmodified.
>
> IOW, I would think the test in question is insisting a single
> outcome for an operation whose result is undefined, and it is
> harmful to twist the system by pessimizing the common cases just
> to cater to this transititory broken state.
>
> I am not saying that we shouldn't have support for users to fix
> their repository and get out of this transititory broken state.  A
> recent work by Torsten Bögershausen to have ls-files report the end
> of line convention used in the blob in the index and the settings
> that affect conversion for each path (among other things) is a step
> in the right direction.  With a support like that, those who noticed
> that they by mistake added CRLF files to the index 

Re: [PATCH 2/2] Fix up the arguments for git stash.

[Junio C Hamano]

Paul Wagland  writes:

> Signed-off-by: Paul Wagland 
> ---

This needs a better explanation than just "Fix up" in the title.
What is broken in the current behaviour and what is the more desired
behaviour?

Thanks.

>  contrib/completion/git-completion.bash | 17 ++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 63754bc..043d5bb 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2382,7 +2382,7 @@ _git_show_branch ()
>  
>  _git_stash ()
>  {
> - local save_opts='--keep-index --no-keep-index --quiet --patch'
> + local save_opts='--all --keep-index --no-keep-index --quiet --patch 
> --include-untracked'
>   local subcommands='save list show apply clear drop pop create branch'
>   local subcommand="$(__git_find_on_cmdline "$subcommands")"
>   if [ -z "$subcommand" ]; then
> @@ -2404,9 +2404,20 @@ _git_stash ()
>   apply,--*|pop,--*)
>   __gitcomp "--index --quiet"
>   ;;
> - show,--*|drop,--*|branch,--*)
> + drop,--*)
> + __gitcomp "--quiet"
>   ;;
> - show,*|apply,*|drop,*|pop,*|branch,*)
> + show,--*|branch,--*)
> + ;;
> + branch,*)
> + if [ $cword -eq 3 ]; then
> + __gitcomp_nl "$(__git_refs)";
> + else
> + __gitcomp_nl "$(git --git-dir="$(__gitdir)" 
> stash list \
> + | sed -n -e 's/:.*//p')"
> + fi
> + ;;
> + show,*|apply,*|drop,*|pop,*)
>   __gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
>   | sed -n -e 's/:.*//p')"
>   ;;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's cooking in git.git (Jan 2016, #02; Mon, 11)

[Junio C Hamano]

Duy Nguyen  writes:

> On Tue, Jan 12, 2016 at 6:45 AM, Junio C Hamano  wrote:
>> * jk/graph-format-padding (2015-09-14) 1 commit
>>  - pretty: pass graph width to pretty formatting for use in '%>|(N)'
>>
>>  Redefine the way '%>|(N)' padding and the "--graph" option
>>  interacts.  It has been that the available columns to display the
>>  log message was measured from the edge of the area the graph ended,
>>  but with this it becomes the beginning of the entire output.
>>
>>  I have a suspicion that 50% of the users would appreciate this
>>  change, and the remainder see this break their expectation.  If
>>  that is the case, we might need to introduce a similar but
>>  different alignment operator so that this new behaviour is
>>  available to those who want to use it, without negatively affecting
>>  existing uses.
>>
>>  No comments after waiting for a long time.
>>  Will discard.
>>  ($gmane/278326)
>
> I carried this in my tree and didn't realize it's dropped from 'pu'.
> There's actually a comment last month [1]. By your last comment, does
> it mean I should check if "it" breaks anything to resurrect this one?
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/277710/focus=282886

Ahh, yes, I remember that your response was that the patch is good
and we do not need a new separate way (as there already is one)
to specify the other behaviour.  So if that is truly the case (I
didn't check it), we would certainly want to resurrect it, as there
wouldn't be any downside to it.

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

Re: [PATCHv3 0/2] Port `git submodule init` from shell to C

[Junio C Hamano]

Stefan Beller  writes:

> This applies on top of sb/submodule-parallel-update, replacing
> sb/submodule-init.
>
> Fixes:
>
>  * a more faithful conversion by staying on stdout (We switch to stderr later
>in another series)
>  * use the existing find_last_dir_sep instead of reinventing the wheel.
>
> Stefan Beller (2):
>   submodule: port resolve_relative_url from shell to C
>   submodule: port init from shell to C

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

Re: PATCH 1/2] Update the flags for git show-branch

[Junio C Hamano]

Thanks.  I'll retitle this to

completion: complete show-branch "--date-order"

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

Re: [PATCH v3 12/15] ref-filter: align: introduce long-form syntax

[Junio C Hamano]

Eric Sunshine  writes:

>> +   git for-each-ref --format="|%(align:$option)refname is 
>> %(refname)%(end)|%(refname)" >actual &&
>
> This is not wrong, per se, but referencing $option inside the
> non-interpolating single-quote context of the test body makes it a bit
> harder to understand than it need be. As it is, at the time that the
> test body actually gets executed, $option still evaluates to the
> desired permutation value so it works. 

I think I said this in one of my recent reviews to somebody else,
but this needs repeating.

My position on this has been and still is a complete opposite.  It
is a designed behaviour that variable references made inside test
body that is quoted with sq pair can see the values of the variables
that are defined outside test_expect_success, and you can freely use
the global $TRASH_DIRECTORY, $_x40, etc. inside the test body that
is quoted with single quote.  The $option here is no different.

In fact, it is more desirable to avoid double-quoting the test body.
The problem with using double-quote around the test body and
formulating the executable shell script using variable interpolation
before test_expect_success even runs is that you would be tempted to
do something silly like this:

HEAD_SHA1=$(git rev-parse --verify HEAD)

test_expect_success "check something" "
HEAD_SHA1=$(git rev-parse --verify HEAD) &&
... do other things that should not move the HEAD ... &&
test '$(git rev-parse HEAD)' = '$HEAD_SHA1' &&
test '$SOME_VAR' = '$OTHER_VAR' &&
"

It is not just error prone (if variable reference had a shell
metacharacter in it, e.g. a single-quote in OTHER_VAR's value, you'd
have a syntax error in the test body), but because two invocations
of rev-parse in the above are both made before test_expect_success
runs, and would yield the same value.  It is not even testing the
right thing.  If you broke rev-parse, the invocation will not fail
inside test_expect_success but outside when the arguments to that
helper is being prepared.

So please do not write something like this:

> test_align_permutations () {
> while ...
> do
> test_expect_success "align:$option" "
> git for-each-ref --format='...$option...' >actual &&
> ...
> "
> done
> }

but make it more like this:

for option in ...
do
test_expect_success "align:$option" '
git for-each-ref --format="...$option..." &&
...
'
done

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

Re: [PATCH 06/19] mingw: try to delete target directory before renaming

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Philip,
>
> On Sun, 24 Jan 2016, Philip Oakley wrote:
>
>> From: "Johannes Schindelin" 
>> >From: 마누엘 
>> 
>> Is this Nalla's preferred email, or just a carry over from cautions of the
>> Github interface?
>
> Neither. It is from the author field as recorded in the commit that I
> merged originally: https://github.com/dscho/git/pull/8

If it is not recorded under the name/email that is preferred by the
author, as I am not pulling from you but will be applying a patch,
we can fix it to match the author's desire, if we wanted to.

Your "Neither" hints that it is the case, but it is unclear to me
what address is the desired one (I can guess hamal.uberspace might
be), so...

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

Re: [PATCH] push --force-with-lease: Fix ref status reporting

[Junio C Hamano]

Andrew Wheeler  writes:

> From: Andrew Wheeler 
>
> The --force--with-lease push option leads to less
> detailed status information than --force. In particular,
> the output indicates that a reference was fast-forwarded,
> even when it was force-updated.
>
> Modify the --force-with-lease ref status logic to leverage
> the --force ref status logic when the "lease" conditions
> are met.

The description of the observed problem makes sense.  Thanks for
working on this.

> Signed-off-by: Andrew Wheeler 
> ---
>  remote.c | 16 +---
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index 9d34b5a..bad6213 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1545,11 +1545,8 @@ void set_ref_status_for_push(struct ref *remote_refs, 
> int send_mirror,
>   }
>  
>   /*
> -  * Bypass the usual "must fast-forward" check but
> -  * replace it with a weaker "the old value must be
> -  * this value we observed".  If the remote ref has
> -  * moved and is now different from what we expect,
> -  * reject any push.
> +  * If the remote ref has moved and is now different
> +  * from what we expect, reject any push.
>*

This simplification of the comment makes sense, especially with the
code that results from the change of the last "else if".

>* It also is an error if the user told us to check
>* with the remote-tracking branch to find the value
> @@ -1560,10 +1557,14 @@ void set_ref_status_for_push(struct ref *remote_refs, 
> int send_mirror,
>   if (ref->expect_old_no_trackback ||
>   oidcmp(>old_oid, >old_oid_expect))
>   reject_reason = REF_STATUS_REJECT_STALE;
> + else
> + /* If the ref isn't stale then force the 
> update. */
> + force_ref_update = 1;
>   }
>  
>   /*
> -  * The usual "must fast-forward" rules.
> +  * If the update isn't already rejected then check
> +  * the usual "must fast-forward" rules.
>*
>* Decide whether an individual refspec A:B can be
>* pushed.  The push will succeed if any of the
> @@ -1580,9 +1581,10 @@ void set_ref_status_for_push(struct ref *remote_refs, 
> int send_mirror,
>*
>* (4) it is forced using the +A:B notation, or by
>* passing the --force argument
> +  *

This new blank line is probably unwanted.

>*/
>  
> - else if (!ref->deletion && !is_null_oid(>old_oid)) {
> + if (!reject_reason && !ref->deletion && 
> !is_null_oid(>old_oid)) {
>   if (starts_with(ref->name, "refs/tags/"))
>   reject_reason = 
> REF_STATUS_REJECT_ALREADY_EXISTS;
>   else if (!has_object_file(>old_oid))

OK.  So the idea is that a successful force-with-lease push would
have a zero reject_reason at this point, and the if/else cascade
would still trigger and would set it to STATUS_REJECT_NEEDS_FORCE,
just like a usual forced push without a lease.  And then because the
local variable force_ref_update is set, it would report the forced
success exactly the same way as the usual forced push.

Sounds very sensible.

Do we want to make sure that other people will not break this fix in
the future by adding a few tests, perhaps to t/t5533?

Thanks.

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

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King  writes:

> I'm not sure "remove-standard-prefix" doesn't open its own questions.
> Like "what are the standard prefixes?".

It is easy to define, no?  This is invented for the internal use of
the listing modes of "tag" and "branch", so the users are welcome to
use it if they see fit, but how the prefixes are stripped is defined
by the convenience of these commands--the behaviour might even change
when these commands are enhanced.

> If we are going to go with "remove a prefix", I really don't think
> "remove if present" is too complicated a set of semantics (as opposed to
> "error out" you mentioned above).
>
> I do like "strip=" for its simplicity (it's easy to explain), and the
> fact that it will probably handle the git-branch case for us. The only
> open question is what to do if there are fewer components, but I really
> think any of the 4 behaviors I gave earlier would be fine.
>
> Eric' globbing suggestion is simpler for the error case (as a prefix, it
> can be "remove if present"), but I think introducing globbing in general
> opens up too many corner cases (e.g., does "*" match "/", is "**"
> supported, etc).

Yeah, I really do not like any of the "do not error out but assume
that the user would not care about the ambiguities" solutions for
something we primarily intend to use for internal purposes.

I agree that "strip=", "remove-prefix=", and the friends
are good for end-user scripting, but they can come later, outside of
the scope of this regression fix, and that is the proper time to
debate and decide if it is really ok to assume that the user does
not care about ambiguity, or it is prudent to error out.  A separate
"remove-standard-prefix" that is meant for internal use would allow
us to push the fix out without having to decide now.

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

Re: [PATCH 2/2] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King  writes:

> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
> git-tag has started showing tags with ambiguous names (i.e.,
> when both "heads/foo" and "tags/foo" exists) as "tags/foo"
> instead of just "foo". This is both:
>
>   - pointless; the output of "git tag" includes only
> refs/tags, so we know that "foo" means the one in
> "refs/tags".
>
> and
>
>   - ambiguous; in the original output, we know that the line
> "foo" means that "refs/tags/foo" exists. In the new
> output, it is unclear whether we mean "refs/tags/foo" or
> "refs/tags/tags/foo".
>
> The reason this happens is that commit b7cc53e9 switched
> git-tag to use ref-filter's "%(refname:short)" output
> formatting, which was adapted from for-each-ref.
> ...

Karthik, getting a fix in for "git tag" regression is more important
than the topics parked in 'pu', so I'll queue this patch in the
early part of 'pu'.

I personally feel that "refname:strip=" would be a good mechanism
for end users to specify a custom format, and it is unclear to me
what should happen when there are not enough elements to be
stripped, so I do not think we want to cast the "we will show the
whole thing" decision in stone prematurely only because we want to
push out the regression fix soon.  So I may ask Jeff to rework this
one (or I may end up trying to do so myself) not to squat on the
nice strip= notation.  refname:strip-standard-prefix that removes
the known prefix ("refs/heads", "refs/remotes" and "refs/tags") if
present and does not touch the refname otherwise would leave us more
time to decide what strip= should do in the error case.

Unfortunately, this means kn/ref-filter-atom-parsing topic from you
that were parked on 'pu' must be ejected for now, as any change in
this area overlaps with it, and the atom parsing code would need to
be updated to learn about the new attribute of the 'refname' atom
(be it 'remove-prefix=', 'strip=', or something else) that
we would decide to use for the regression fix anyway.

Thanks.

>  Documentation/git-for-each-ref.txt |  6 +-
>  Documentation/git-tag.txt  |  2 +-
>  builtin/tag.c  |  4 ++--
>  ref-filter.c   | 13 -
>  t/t3203-branch-output.sh   |  8 
>  t/t6300-for-each-ref.sh|  4 
>  t/t7004-tag.sh |  8 
>  7 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt 
> b/Documentation/git-for-each-ref.txt
> index 06208c4..f15c817 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,7 +92,11 @@ refname::
>   The name of the ref (the part after $GIT_DIR/).
>   For a non-ambiguous short name of the ref append `:short`.
>   The option core.warnAmbiguousRefs is used to select the strict
> - abbreviation mode.
> + abbreviation mode. If `strip=` is appended, strips ``
> + slash-separated path components from the front of the refname
> + (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> + If the ref has fewer components than ``, the whole,
> + unstripped `%(refname)` is printed.
>  
>  objecttype::
>   The type of the object (`blob`, `tree`, `commit`, `tag`).
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 7220e5e..abab481 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -163,7 +163,7 @@ This option is only applicable when listing tags without 
> annotation lines.
>   A string that interpolates `%(fieldname)` from the object
>   pointed at by a ref being shown.  The format is the same as
>   that of linkgit:git-for-each-ref[1].  When unspecified,
> - defaults to `%(refname:short)`.
> + defaults to `%(refname:strip=2)`.
>  
>  --[no-]merged []::
>   Only list tags whose tips are reachable, or not reachable
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 8db8c87..1705c94 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct 
> ref_sorting *sorting, con
>   if (!format) {
>   if (filter->lines) {
>   to_free = xstrfmt("%s %%(contents:lines=%d)",
> -   "%(align:15)%(refname:short)%(end)",
> +   "%(align:15)%(refname:strip=2)%(end)",
> filter->lines);
>   format = to_free;
>   } else
> - format = "%(refname:short)";
> + format = "%(refname:strip=2)";
>   }
>  
>   verify_ref_format(format);
> diff --git a/ref-filter.c b/ref-filter.c
> index 7bef7f8..9f54adc 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -909,12 +909,23 @@ static void populate_value(struct ref_array_item *ref)
>   formatp = strchr(name, ':');
>

Re: [PATCH v3 13/15] ref-filter: introduce remote_ref_atom_parser()

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak  wrote:
> Introduce remote_ref_atom_parser() which will parse the '%(upstream)'
> and '%(push)' atoms and store information into the 'used_atom'
> structure based on the modifiers used along with the corresponding
> atom.
>
> Helped-by: Ramsay Jones 
> Helped-by: Eric Sunshine 
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -841,6 +863,43 @@ static inline char *copy_advance(char *dst, const char 
> *src)
> +static void fill_remote_ref_details(struct used_atom *atom, const char 
> *refname,
> +   struct branch *branch, const char **s)
> +{
> +   int num_ours, num_theirs;
> +   if (atom->u.remote_ref == RR_SHORTEN)
> +   *s = shorten_unambiguous_ref(refname, warn_ambiguous_refs);
> +   else if (atom->u.remote_ref == RR_TRACK) {
> +   if (stat_tracking_info(branch, _ours,
> +  _theirs, NULL))
> +   return;
> +
> +   if (!num_ours && !num_theirs)
> +   *s = "";
> +   else if (!num_ours)
> +   *s = xstrfmt("[behind %d]", num_theirs);
> +   else if (!num_theirs)
> +   *s = xstrfmt("[ahead %d]", num_ours);
> +   else
> +   *s = xstrfmt("[ahead %d, behind %d]",
> +num_ours, num_theirs);
> +   } else if (atom->u.remote_ref == RR_TRACKSHORT) {
> +   if (stat_tracking_info(branch, _ours,
> +  _theirs, NULL))
> +   return;
> +
> +   if (!num_ours && !num_theirs)
> +   *s = "=";
> +   else if (!num_ours)
> +   *s = "<";
> +   else if (!num_theirs)
> +   *s = ">";
> +   else
> +   *s = "<>";
> +   } else if (atom->u.remote_ref == RR_NORMAL)
> +   *s = refname;

I think I mentioned this in a previous review: If the code falls past
this final 'else if' for some reason (programmer error), then *s won't
get assigned at all, which is probably undesirable. To protect against
such a case, you might want either to add a final 'else':

else
die("BUG: ...");

or just consider RR_NORMAL the catchall case, and turn the final 'else
if' into a plain 'else':

else /* RR_NORMAL */
*s = refname;

> +}
> @@ -894,6 +953,8 @@ static void populate_value(struct ref_array_item *ref)
> refname = branch_get_upstream(branch, NULL);
> if (!refname)
> continue;
> +   fill_remote_ref_details(atom, refname, branch, >s);
> +   continue;

There are now two 'continue' statements very close together here. Have
you considered this instead?

if (refname)
fill_remote_ref_details(...);
continue;

It might make the code a bit more straightforward. (Genuine question;
I don't feel too strongly about it.)

> } else if (starts_with(name, "push")) {
> const char *branch_name;
> if (!skip_prefix(ref->refname, "refs/heads/",
> @@ -904,6 +965,8 @@ static void populate_value(struct ref_array_item *ref)
> refname = branch_get_push(branch, NULL);
> if (!refname)
> continue;
> +   fill_remote_ref_details(atom, refname, branch, >s);
> +   continue;

Ditto.

> } else if (starts_with(name, "color:")) {
> v->s = atom->u.color;
> continue;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 0/2] Let merge-file write out conflict markers with correct EOLs

[Johannes Schindelin]

The original patch was sent by Beat Bolli in
http://thread.gmane.org/gmane.comp.version-control.git/281600

My suggestion to extend it to respect gitattributes led to
changes that broke the original patch. And they were misguided
to begin with (see below).

Since there have been a couple of "What's cooking" mails
containing unheard calls for updates on this patch, I took it
on myself to fix things.

Junio's comment that we might look at blobs containing CR/LF
line endings made me rethink the entire approach, and I am now
convinced that we need to abandon the entire idea to make the
conflict markers depend on config settings or attributes:
after all, I introduced `git merge-file` as a replacement for
GNU merge that can be used *outside* of any repository, by design.

The crucial idea hit me yesterday when I took a step back: all
we need to do is to ensure that the end-of-line style is matched
when *all* input files are LF-only, or when they are all CR/LF.
In all other cases, we have mixed line endings anyway.

And to do that, it is sufficient to look at *one single line
ending* in the context. Technically, it does not even have to be
the context, but the first line endings of the first file would
be enough, however it is so much more pleasant if the conflict
marker's eol matches the one of the preceding line.

To prevent my future self from repeating mistakes, I added a
little bit of background to the first commit message.

Triggered by a comment by Junio, I realized that a second patch is
needed: we need to make sure that the conflicting sections are also
augmented by the appropriate line endings if they lack any.

Relative to v2, the first patch changed only in the test code, to
accomodate for the newly-introduced second patch.


Johannes Schindelin (2):
  merge-file: let conflict markers match end-of-line style of the
context
  merge-file: ensure that conflict sections match eol style

 t/t6023-merge-file.sh | 13 +++
 xdiff/xmerge.c| 98 +--
 2 files changed, 93 insertions(+), 18 deletions(-)

Interdiff vs v2:

 diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
 index f0cb9ce..1390548 100755
 --- a/t/t6023-merge-file.sh
 +++ b/t/t6023-merge-file.sh
 @@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved 
by --union' \
 printf "line1\nline2\nline3x\nline3y" >expect.txt &&
 test_cmp expect.txt output.txt'
  
 -test_expect_success 'conflict markers match existing line endings' '
 -  append_cr crlf-orig.txt &&
 -  append_cr crlf-diff1.txt &&
 -  append_cr crlf-diff2.txt &&
 +test_expect_success 'conflict sections match existing line endings' '
 +  printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
 +  printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
 +  printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
test_must_fail git -c core.eol=crlf merge-file -p \
crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
test $(tr "\015" Q nolf.txt &&
test $(tr "\015" Q size;
if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
 +  if (needs_cr) {
 +  if (dest)
 +  dest[size] = '\r';
 +  size++;
 +  }
 +
if (dest)
dest[size] = '\n';
size++;
 @@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, 
int i, int count, int add
return size;
  }
  
 -static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char 
*dest)
 +static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int 
add_nl, char *dest)
  {
 -  return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
 +  return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
  }
  
 -static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char 
*dest)
 +static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int 
add_nl, char *dest)
  {
 -  return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
 +  return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
  }
  
  /*
 @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
file->recs[i - 1]->ptr[size - 2] == '\r';
  }
  
 -static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
 -xdfenv_t *xe2, const char *name2,
 -const char *name3,
 -int size, int i, int style,
 -xdmerge_t *m, char *dest, int marker_size)
 +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
  {
 -  int marker1_size = (name1 ? strlen(name1) + 1 : 0);
 -  int marker2_size = (name2 ? strlen(name2) + 1 : 0);
 -  int marker3_size = (name3 ? strlen(name3) + 1 : 0);
int needs_cr;
  
/* 

[PATCH v3 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

In the previous patch, we made sure that the conflict markers themselves
match the end-of-line style of the input files. However, this still left
out the conflicting text itself: if it lacks a trailing newline, we
add one, and should add a carriage return when appropriate, too.

Signed-off-by: Johannes Schindelin 
---
 t/t6023-merge-file.sh |  3 ++-
 xdiff/xmerge.c| 61 +++
 2 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index bb20cbc..1390548 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,13 +346,14 @@ test_expect_success 'conflict at EOF without LF resolved 
by --union' \
 printf "line1\nline2\nline3x\nline3y" >expect.txt &&
 test_cmp expect.txt output.txt'
 
-test_expect_success 'conflict markers match existing line endings' '
+test_expect_success 'conflict sections match existing line endings' '
printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
test_must_fail git -c core.eol=crlf merge-file -p \
crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
test $(tr "\015" Q nolf.txt &&
test $(tr "\015" Q size;
if (i == 0 || recs[count - 1]->ptr[i - 1] != '\n') {
+   if (needs_cr) {
+   if (dest)
+   dest[size] = '\r';
+   size++;
+   }
+
if (dest)
dest[size] = '\n';
size++;
@@ -133,14 +139,14 @@ static int xdl_recs_copy_0(int use_orig, xdfenv_t *xe, 
int i, int count, int add
return size;
 }
 
-static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int add_nl, char 
*dest)
+static int xdl_recs_copy(xdfenv_t *xe, int i, int count, int needs_cr, int 
add_nl, char *dest)
 {
-   return xdl_recs_copy_0(0, xe, i, count, add_nl, dest);
+   return xdl_recs_copy_0(0, xe, i, count, needs_cr, add_nl, dest);
 }
 
-static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int add_nl, char 
*dest)
+static int xdl_orig_copy(xdfenv_t *xe, int i, int count, int needs_cr, int 
add_nl, char *dest)
 {
-   return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
+   return xdl_recs_copy_0(1, xe, i, count, needs_cr, add_nl, dest);
 }
 
 /*
@@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
file->recs[i - 1]->ptr[size - 2] == '\r';
 }
 
-static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
- xdfenv_t *xe2, const char *name2,
- const char *name3,
- int size, int i, int style,
- xdmerge_t *m, char *dest, int marker_size)
+static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
 {
-   int marker1_size = (name1 ? strlen(name1) + 1 : 0);
-   int marker2_size = (name2 ? strlen(name2) + 1 : 0);
-   int marker3_size = (name3 ? strlen(name3) + 1 : 0);
int needs_cr;
 
/* Match post-images' preceding, or first, lines' end-of-line style */
@@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
*name1,
if (needs_cr)
needs_cr = is_eol_crlf(>xdf1, 0);
/* If still undecided, use LF-only */
-   if (needs_cr < 0)
-   needs_cr = 0;
+   return needs_cr < 0 ? 0 : needs_cr;
+}
+
+static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
+ xdfenv_t *xe2, const char *name2,
+ const char *name3,
+ int size, int i, int style,
+ xdmerge_t *m, char *dest, int marker_size)
+{
+   int marker1_size = (name1 ? strlen(name1) + 1 : 0);
+   int marker2_size = (name2 ? strlen(name2) + 1 : 0);
+   int marker3_size = (name3 ? strlen(name3) + 1 : 0);
+   int needs_cr = is_cr_needed(xe1, xe2, m);
 
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
 
/* Before conflicting part */
-   size += xdl_recs_copy(xe1, i, m->i1 - i, 0,
+   size += xdl_recs_copy(xe1, i, m->i1 - i, 0, 0,
  dest ? dest + size : NULL);
 
if (!dest) {
@@ -217,7 +227,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
*name1,
}
 
/* Postimage from side #1 */
-   size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
+   size += xdl_recs_copy(xe1, m->i1, m->chg1, needs_cr, 1,
  dest ? dest + size : NULL);
 
if (style == XDL_MERGE_DIFF3) {
@@ -236,7 +246,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
*name1,
dest[size++] = '\r';

[PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

When merging files with CR/LF line endings, the conflict markers should
match those, lest the output file has mixed line endings.

This is particularly of interest on Windows, where some editors get
*really* confused by mixed line endings.

The original version of this patch by Beat Bolli respected core.eol, and
a subsequent improvement by this developer also respected gitattributes.
This approach was suboptimal, though: `git merge-file` was invented as a
drop-in replacement for GNU merge and as such has no problem operating
outside of any repository at all!

Another problem with the original approach was pointed out by Junio
Hamano: legacy repositories might have their text files committed using
CR/LF line endings (and core.eol and the gitattributes would give us a
false impression there). Therefore, the much superior approach is to
simply match the context's line endings, if any.

We actually do not have to look at the *entire* context at all: if the
files are all LF-only, or if they all have CR/LF line endings, it is
sufficient to look at just a *single* line to match that style. And if
the line endings are mixed anyway, it is *still* okay to imitate just a
single line's eol: we will just add to the pile of mixed line endings,
and there is nothing we can do about that.

So what we do is: we look at the line preceding the conflict, falling
back to the line preceding that in case it was the last line and had no
line ending, falling back to the first line, first in the first
post-image, then the second post-image, and finally the pre-image.
If we find consistent CR/LF (or undecided) end-of-line style, we match
that, otherwise we use LF-only line endings for the conflict markers.

Note that while it is true that there have to be at least two lines we
can look at (otherwise there would be no conflict), the same is not true
for line *endings*: the three files in question could all consist of a
single line without any line ending, each. In this case we fall back to
using LF-only.

Signed-off-by: Johannes Schindelin 
---
 t/t6023-merge-file.sh | 12 +++
 xdiff/xmerge.c| 57 +++
 2 files changed, 65 insertions(+), 4 deletions(-)

diff --git a/t/t6023-merge-file.sh b/t/t6023-merge-file.sh
index 190ee90..bb20cbc 100755
--- a/t/t6023-merge-file.sh
+++ b/t/t6023-merge-file.sh
@@ -346,4 +346,16 @@ test_expect_success 'conflict at EOF without LF resolved 
by --union' \
 printf "line1\nline2\nline3x\nline3y" >expect.txt &&
 test_cmp expect.txt output.txt'
 
+test_expect_success 'conflict markers match existing line endings' '
+   printf "1\\r\\n2\\r\\n3" >crlf-orig.txt &&
+   printf "1\\r\\n2\\r\\n4" >crlf-diff1.txt &&
+   printf "1\\r\\n2\\r\\n5" >crlf-diff2.txt &&
+   test_must_fail git -c core.eol=crlf merge-file -p \
+   crlf-diff1.txt crlf-orig.txt crlf-diff2.txt >crlf.txt &&
+   test $(tr "\015" Q nolf.txt &&
+   test $(tr "\015" Q nrec - 1)
+   /* All lines before the last *must* end in LF */
+   return (size = file->recs[i]->size) > 1 &&
+   file->recs[i]->ptr[size - 2] == '\r';
+   if (!file->nrec)
+   /* Cannot determine eol style from empty file */
+   return -1;
+   if ((size = file->recs[i]->size) &&
+   file->recs[i]->ptr[size - 1] == '\n')
+   /* Last line; ends in LF; Is it CR/LF? */
+   return size > 1 &&
+   file->recs[i]->ptr[size - 2] == '\r';
+   if (!i)
+   /* The only line has no eol */
+   return -1;
+   /* Determine eol from second-to-last line */
+   return (size = file->recs[i - 1]->size) > 1 &&
+   file->recs[i - 1]->ptr[size - 2] == '\r';
+}
+
 static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
  xdfenv_t *xe2, const char *name2,
  const char *name3,
@@ -152,6 +181,18 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
*name1,
int marker1_size = (name1 ? strlen(name1) + 1 : 0);
int marker2_size = (name2 ? strlen(name2) + 1 : 0);
int marker3_size = (name3 ? strlen(name3) + 1 : 0);
+   int needs_cr;
+
+   /* Match post-images' preceding, or first, lines' end-of-line style */
+   needs_cr = is_eol_crlf(>xdf2, m->i1 ? m->i1 - 1 : 0);
+   if (needs_cr)
+   needs_cr = is_eol_crlf(>xdf2, m->i2 ? m->i2 - 1 : 0);
+   /* Look at pre-image's first line, unless we already settled on LF */
+   if (needs_cr)
+   needs_cr = is_eol_crlf(>xdf1, 0);
+   /* If still undecided, use LF-only */
+   if (needs_cr < 0)
+   needs_cr = 0;
 
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -161,7 +202,7 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
*name1,
   

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Torsten Bögershausen]

On 25.01.16 09:07, Johannes Schindelin wrote:
[]
> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 625198e..c852acc 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -143,6 +143,35 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int count, 
> int add_nl, char *dest)
>   return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
>  }
>  
> +/*
> + * Returns 1 if the i'th line ends in CR/LF (if it is the last line and
> + * has no eol, the preceding line, if any), 0 if it ends in LF-only, and
> + * -1 if the line ending cannot be determined.
> + */
> +static int is_eol_crlf(xdfile_t *file, int i)
Minot nit: Could that be 
is_eol_crlf(xdfile_t *file, int lineno)
(or may be)
is_eol_crlf(const xdfile_t *file, int lineno)
(or may be)
has_eol_crlf(const xdfile_t *file, int lineno)

> +{
> + long size;
> +
> + if (i < file->nrec - 1)
> + /* All lines before the last *must* end in LF */
> + return (size = file->recs[i]->size) > 1 &&
> + file->recs[i]->ptr[size - 2] == '\r';
> + if (!file->nrec)
> + /* Cannot determine eol style from empty file */
> + return -1;
> + if ((size = file->recs[i]->size) &&
> + file->recs[i]->ptr[size - 1] == '\n')
> + /* Last line; ends in LF; Is it CR/LF? */
> + return size > 1 &&
> + file->recs[i]->ptr[size - 2] == '\r';
> + if (!i)
> + /* The only line has no eol */
> + return -1;
> + /* Determine eol from second-to-last line */
> + return (size = file->recs[i - 1]->size) > 1 &&
> + file->recs[i - 1]->ptr[size - 2] == '\r';
> +}
> +

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

Re: [PATCH v3 2/2] merge-file: ensure that conflict sections match eol style

[Johannes Schindelin]

Hi,

On Mon, 25 Jan 2016, Johannes Schindelin wrote:

> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index c852acc..d98f430 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -172,15 +178,8 @@ static int is_eol_crlf(xdfile_t *file, int i)
>   file->recs[i - 1]->ptr[size - 2] == '\r';
>  }
>  
> -static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
> -   xdfenv_t *xe2, const char *name2,
> -   const char *name3,
> -   int size, int i, int style,
> -   xdmerge_t *m, char *dest, int marker_size)
> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
>  {
> - int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> - int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> - int marker3_size = (name3 ? strlen(name3) + 1 : 0);
>   int needs_cr;
>  
>   /* Match post-images' preceding, or first, lines' end-of-line style */
> @@ -191,14 +190,25 @@ static int fill_conflict_hunk(xdfenv_t *xe1, const char 
> *name1,
>   if (needs_cr)
>   needs_cr = is_eol_crlf(>xdf1, 0);
>   /* If still undecided, use LF-only */
> - if (needs_cr < 0)
> - needs_cr = 0;
> + return needs_cr < 0 ? 0 : needs_cr;
> +}
> +
> +static int fill_conflict_hunk(xdfenv_t *xe1, const char *name1,
> +   xdfenv_t *xe2, const char *name2,
> +   const char *name3,
> +   int size, int i, int style,
> +   xdmerge_t *m, char *dest, int marker_size)
> +{
> + int marker1_size = (name1 ? strlen(name1) + 1 : 0);
> + int marker2_size = (name2 ? strlen(name2) + 1 : 0);
> + int marker3_size = (name3 ? strlen(name3) + 1 : 0);
> + int needs_cr = is_cr_needed(xe1, xe2, m);
>  
>   if (marker_size <= 0)
>   marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
>  

Oh bummer. I just realized that I should have refactored that already in
patch 1/2 before sending out v3. Of course it would be true to history to
do the refactoring only as part of 2/2, but who cares about true history
when one can rewrite it?

Will send out v4 in a while (I want to wait for more feedback in case I
need to change more things.) In the meantime, you can always look at my
patch series' current state here:

https://github.com/git/git/compare/next...dscho:merge-marker-crlf

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

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 06:39:05PM -0500, Eric Sunshine wrote:

> > Yeah, "strip=2" would also get the job done, and extends more naturally
> > to the branch case.
> >
> > To be honest, I cannot imagine anybody using anything _but_ strip=2, but
> > maybe there are special cases, like:
> >
> >   git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/
> >
> > to get my list of topics, sans initials.
> 
> What if the option was named ":stripprefix=" in its most general form:
> 
> %(refname:stripprefix=refs/tags/)
> 
> with plain:
> 
> %(refname:stripprefix)
> 
> shorthand for ":stripprefix=refs/*/" or something?

That would work, though I was really hoping not to get into something as
complicated as globbing. I'm not sure it really buys us that much here.

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

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Yeah, "strip=2" would also get the job done, and extends more naturally
> > to the branch case.
> >
> > To be honest, I cannot imagine anybody using anything _but_ strip=2...
> 
> I 100% agree, and I do consider this to be internal implementation
> detail for the listing modes of "tag" (and "branch"), which may be
> exposed to the user (by documenting that %(refname:X) is used by
> default), so perhaps even the flexibility of strip=2 is unwanted.
> 
> I know what "remove-standard-prefix" is way too long for the value
> of X above, but then we can say "the command will error out if you
> allow your for-each-ref invocation to step outside of the area that
> has standard prefix to be removed." without having to worry about
> "what is the sensible thing to do when the prefixes are not what we
> expect (too short for strip=2 or no match for short=refs/tags/)".

I'm not sure "remove-standard-prefix" doesn't open its own questions.
Like "what are the standard prefixes?".

If we are going to go with "remove a prefix", I really don't think
"remove if present" is too complicated a set of semantics (as opposed to
"error out" you mentioned above).

I do like "strip=" for its simplicity (it's easy to explain), and the
fact that it will probably handle the git-branch case for us. The only
open question is what to do if there are fewer components, but I really
think any of the 4 behaviors I gave earlier would be fine.

Eric' globbing suggestion is simpler for the error case (as a prefix, it
can be "remove if present"), but I think introducing globbing in general
opens up too many corner cases (e.g., does "*" match "/", is "**"
supported, etc).

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

Re: [PATCH v3 1/2] merge-file: let conflict markers match end-of-line style of the context

[Johannes Schindelin]

Hi Torsten,

On Mon, 25 Jan 2016, Torsten Bögershausen wrote:

> On 25.01.16 09:07, Johannes Schindelin wrote:
> []
> > diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> > index 625198e..c852acc 100644
> > --- a/xdiff/xmerge.c
> > +++ b/xdiff/xmerge.c
> > @@ -143,6 +143,35 @@ static int xdl_orig_copy(xdfenv_t *xe, int i, int 
> > count, int add_nl, char *dest)
> > return xdl_recs_copy_0(1, xe, i, count, add_nl, dest);
> >  }
> >  
> > +/*
> > + * Returns 1 if the i'th line ends in CR/LF (if it is the last line and
> > + * has no eol, the preceding line, if any), 0 if it ends in LF-only, and
> > + * -1 if the line ending cannot be determined.
> > + */
> > +static int is_eol_crlf(xdfile_t *file, int i)
> Minot nit: Could that be 
> is_eol_crlf(xdfile_t *file, int lineno)
> (or may be)
> is_eol_crlf(const xdfile_t *file, int lineno)
> (or may be)
> has_eol_crlf(const xdfile_t *file, int lineno)

The surrounding code consistently uses i, and not lineno. The reason (I
guess) is that i starts at 0 whereas lineno would have to start at 1, and
we can easily avoid adding and subtracting 1 all the time.

Ciao,
Dscho

Re: What's cooking in git.git (Jan 2016, #02; Mon, 11)

[Duy Nguyen]

On Tue, Jan 12, 2016 at 6:45 AM, Junio C Hamano  wrote:
> * jk/graph-format-padding (2015-09-14) 1 commit
>  - pretty: pass graph width to pretty formatting for use in '%>|(N)'
>
>  Redefine the way '%>|(N)' padding and the "--graph" option
>  interacts.  It has been that the available columns to display the
>  log message was measured from the edge of the area the graph ended,
>  but with this it becomes the beginning of the entire output.
>
>  I have a suspicion that 50% of the users would appreciate this
>  change, and the remainder see this break their expectation.  If
>  that is the case, we might need to introduce a similar but
>  different alignment operator so that this new behaviour is
>  available to those who want to use it, without negatively affecting
>  existing uses.
>
>  No comments after waiting for a long time.
>  Will discard.
>  ($gmane/278326)

I carried this in my tree and didn't realize it's dropped from 'pu'.
There's actually a comment last month [1]. By your last comment, does
it mean I should check if "it" breaks anything to resurrect this one?

[1] http://thread.gmane.org/gmane.comp.version-control.git/277710/focus=282886
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] push --force-with-lease: Fix ref status reporting

[Andrew Wheeler]

On Mon, Jan 25, 2016 at 1:37 PM, Junio C Hamano  wrote:

> >* passing the --force argument
> > +  *
>
> This new blank line is probably unwanted.


> Do we want to make sure that other people will not break this fix in
> the future by adding a few tests, perhaps to t/t5533?
>
> Thanks.
>

Great idea. I'll add some tests to t/t5533, remove that blank line
above, and share a new patch.

Thanks for the review!

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

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Mon, Jan 25, 2016 at 12:21:21PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > I'm not sure "remove-standard-prefix" doesn't open its own questions.
> > Like "what are the standard prefixes?".
> 
> It is easy to define, no?  This is invented for the internal use of
> the listing modes of "tag" and "branch", so the users are welcome to
> use it if they see fit, but how the prefixes are stripped is defined
> by the convenience of these commands--the behaviour might even change
> when these commands are enhanced.

What does this do:

  git for-each-ref --format='%(refname:remove-standard-prefix)'

?

Is there no standard prefix, because we are not in branch/tag? Does it
remove well-known prefixes like "refs/heads/", "refs/tags/", etc? Does
it remove the first two components (e.g., what happens to
"refs/foo/bar")? If so, what happens to "refs/stash"?

We can say "it is all internally defined and subject to change". But
that is not very helpful to a user, and this "it's magic, don't rely on
it" wart will be part of the user-facing interface. We have to write
_something_ in the "default format" documentation for git-tag.

> Yeah, I really do not like any of the "do not error out but assume
> that the user would not care about the ambiguities" solutions for
> something we primarily intend to use for internal purposes.
> 
> I agree that "strip=", "remove-prefix=", and the friends
> are good for end-user scripting, but they can come later, outside of
> the scope of this regression fix, and that is the proper time to
> debate and decide if it is really ok to assume that the user does
> not care about ambiguity, or it is prudent to error out.  A separate
> "remove-standard-prefix" that is meant for internal use would allow
> us to push the fix out without having to decide now.

Yeah, I am definitely on board with trying to do the most minimal thing
for the regression fix and worry about more advanced concerns later (if
at all). It seems to me that "error out" is a pretty minimal behavior,
though, and one that doesn't lock us into any behaviors (i.e., it is
generally OK to take something that did not work at all before, and give
it new useful behavior; it is not OK to change something that did
something useful before).

So what about this:

  1. Implement ":strip=" and use it from git-tag.

  2. Have it error out on a ref with fewer than  components. This
 should be impossible to trigger via "git-tag" with the default
 format.

  3. Explicitly document that the behavior for values of  that are
 not positive integers is undefined and subject to change (or
 alternatively, we can simply error out).

That _is_ user-visible, but it seems like "strip" is a fairly flexible
mechanism by itself (enough that I do not mind living with it forever),
and we haven't made any promises about the ambiguous parts.


If we are going to do something _really_ internal and undocumented to
fix the regression, I think I would rather not introduce a new formatter
at all, but simply teach "%(refname:short)" to do the magic internal
shortening in the context of git-tag. That does not let people emulate
"tag" with "for-each-ref", but that is not part of the regression or its
fix.

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

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Mon, Jan 25, 2016 at 09:37:53PM -0500, Jeff King wrote:

> So what about this:
> 
>   1. Implement ":strip=" and use it from git-tag.
> 
>   2. Have it error out on a ref with fewer than  components. This
>  should be impossible to trigger via "git-tag" with the default
>  format.
> 
>   3. Explicitly document that the behavior for values of  that are
>  not positive integers is undefined and subject to change (or
>  alternatively, we can simply error out).
> 
> That _is_ user-visible, but it seems like "strip" is a fairly flexible
> mechanism by itself (enough that I do not mind living with it forever),
> and we haven't made any promises about the ambiguous parts.

Here's the matching patch. It's "v3 2/2", where "v3 1/2" is the same
test-cleanup patch I posted earlier.

-- >8 --
Subject: [PATCH v2 2/2] tag: do not show ambiguous tag names as "tags/foo"

Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
git-tag has started showing tags with ambiguous names (i.e.,
when both "heads/foo" and "tags/foo" exists) as "tags/foo"
instead of just "foo". This is both:

  - pointless; the output of "git tag" includes only
refs/tags, so we know that "foo" means the one in
"refs/tags".

and

  - ambiguous; in the original output, we know that the line
"foo" means that "refs/tags/foo" exists. In the new
output, it is unclear whether we mean "refs/tags/foo" or
"refs/tags/tags/foo".

The reason this happens is that commit b7cc53e9 switched
git-tag to use ref-filter's "%(refname:short)" output
formatting, which was adapted from for-each-ref. This more
general code does not know that we care only about tags, and
uses shorten_unambiguous_ref to get the short-name. We need
to tell it that we care only about "refs/tags/", and it
should shorten with respect to that value.

In theory, the ref-filter code could figure this out by us
passing FILTER_REFS_TAGS. But there are two complications
there:

  1. The handling of refname:short is deep in formatting
 code that does not even have our ref_filter struct, let
 alone the arguments to the filter_ref struct.

  2. In git v2.7.0, we expose the formatting language to the
 user. If we follow this path, it will mean that
 "%(refname:short)" behaves differently for "tag" versus
 "for-each-ref" (including "for-each-ref refs/tags/"),
 which can lead to confusion.

Instead, let's add a new modifier to the formatting
language, "strip", to remove a specific set of prefix
components. This fixes "git tag", and lets users invoke the
same behavior from their own custom formats (for "tag" or
"for-each-ref") while leaving ":short" with its same
consistent meaning in all places.

We introduce a test in t7004 for "git tag", which fails
without this patch. We also add a similar test in t3203 for
"git branch", which does not actually fail. But since it is
likely that "branch" will eventually use the same formatting
code, the test helps defend against future regressions.

Signed-off-by: Jeff King 
---
 Documentation/git-for-each-ref.txt |  6 +-
 Documentation/git-tag.txt  |  2 +-
 builtin/tag.c  |  4 ++--
 ref-filter.c   | 26 ++
 t/t3203-branch-output.sh   |  8 
 t/t6300-for-each-ref.sh| 12 
 t/t7004-tag.sh |  8 
 7 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt 
b/Documentation/git-for-each-ref.txt
index 06208c4..2e3e96f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,11 @@ refname::
The name of the ref (the part after $GIT_DIR/).
For a non-ambiguous short name of the ref append `:short`.
The option core.warnAmbiguousRefs is used to select the strict
-   abbreviation mode.
+   abbreviation mode. If `strip=` is appended, strips ``
+   slash-separated path components from the front of the refname
+   (e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
+   `` must be a positive integer.  If a displayed ref has fewer
+   components than ``, the command aborts with an error.
 
 objecttype::
The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7220e5e..abab481 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -163,7 +163,7 @@ This option is only applicable when listing tags without 
annotation lines.
A string that interpolates `%(fieldname)` from the object
pointed at by a ref being shown.  The format is the same as
that of linkgit:git-for-each-ref[1].  When unspecified,
-   defaults to `%(refname:short)`.
+   defaults to `%(refname:strip=2)`.
 
 --[no-]merged []::
Only list tags whose tips are reachable, or not reachable
diff --git a/builtin/tag.c b/builtin/tag.c
index 

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King  writes:

> What does this do:
>
>   git for-each-ref --format='%(refname:remove-standard-prefix)'
>
> ?
>
> Is there no standard prefix, because we are not in branch/tag? Does it
> remove well-known prefixes like "refs/heads/", "refs/tags/", etc? Does
> it remove the first two components (e.g., what happens to
> "refs/foo/bar")? If so, what happens to "refs/stash"?

I think our mails crossed.  I envisioned that the documentation
would go something like this:

remove-standard-prefix::
When the refname begins with one of the well-known prefixes,
the prefix is stripped from it (and when it does not start
with any of the well-known prefixes, the refname is left
as-is).  "refs/heads/", "refs/remotes/" and "refs/tags/" are
the well-known prefixes that are removed (as this modifier
is primarily meant to allow users emulate the listing modes
of "git tag" and "git branch") but this set may change over
time (we may teach it to remove "refs/changes/" in later
versions of Git, for example).

> Yeah, I am definitely on board with trying to do the most minimal thing
> for the regression fix and worry about more advanced concerns later (if
> at all). It seems to me that "error out" is a pretty minimal behavior,
> though, and one that doesn't lock us into any behaviors (i.e., it is
> generally OK to take something that did not work at all before, and give
> it new useful behavior; it is not OK to change something that did
> something useful before).

The thing that worried me the most is that "strip=" is a very
intuitive and nice notation that end users would want to use it
beyond emulating "git tag" literally, and one behaviour we happen to
pick, be it "error out" or "leave it intact", would have a high
chance of being not the most useful one.  If we define it to error
out, somebody somewhere will abuse it to "ensure that all branch
names are at least two levels deep" or something we do not
anticipate now and start relying on the "erroring out" behaviour,
and then complain when we later "allow it to do something more
useful" that it no longer errors out.

Having said all that, I think I can live with "strip=" that
happens to pick a single behaviour that we pick with the best
knowledge we have right now.  If we fear the compatiblity wart so
seriously, we can always add a separate "strip-nofail=" variant
that gives a different behaviour from "strip=" that errors out.

Another useful variant might be "strip  levels if we can,
otherwise pretend that the ref did not even pass the filtering
criteria and do not show anything about the ref on the output", by
the way.

> So what about this:
>
>   1. Implement ":strip=" and use it from git-tag.
>
>   2. Have it error out on a ref with fewer than  components. This
>  should be impossible to trigger via "git-tag" with the default
>  format.
>
>   3. Explicitly document that the behavior for values of  that are
>  not positive integers is undefined and subject to change (or
>  alternatively, we can simply error out).
>
>
> That _is_ user-visible, but it seems like "strip" is a fairly flexible
> mechanism by itself (enough that I do not mind living with it forever),
> and we haven't made any promises about the ambiguous parts.

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