[no subject]

[Roxana Guerrero Nunez]

Herzlichen Glückwunsch, Sie haben € 650.000,00 bei den monatlichen 
Gewinnspielen von Euro Millions / Google Promo am März 2018 gewonnen. 
Kontaktieren Sie unseren Schadenversicherer E-Mail: eurosilli...@gmail.com

1. Vollständiger Name:
2. Adresse:
3. Sex:
4. Alter:
5. Beruf:
6. Telefon:

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 13 March 2018 at 22:46, Junio C Hamano  wrote:
> Martin Ågren  writes:
>
>> in order to decide whether we should read from stdin. (So yes, after
>> this patch, we will still silently ignore stdin for confused usage such
>> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
>> not crash.)
>
> $ git log -p | git shortlog Documentation/
>
> is also a nonsense request.  When outside a repository, i.e.
>
> $ git -C $path_to_repo | git shortlog Documentation/
>
> is not giving any revisions, so the error message should not say "no
> revisions can be given"---a nitpicky bug reporter would say "I gave
> no revisions, why are you complaining to me?"

Good point. I will see if I can make this similar to other places. Maybe
something like "too many arguments given outside repository". Thanks.

Martin

[GSoC][PATCH 1/2] git-ci: add .pylintrc file

[Viet Hung Tran]

Hello Mr. Diamand,

I have added the .pylintrc configuration file into the repository.
It still reports several warnings as you said. 

Here is the link for my build: 
https://travis-ci.org/VietHTran/git/jobs/353135582

Do you think I should edit the .pylintrc in order for it to also ignore the 
current warning? I included that edit in the [PATCH 2/2]. 

Viet

Signed-off-by: Viet Hung Tran 
---
 .pylintrc | 577 ++
 1 file changed, 577 insertions(+)
 create mode 100644 .pylintrc

diff --git a/.pylintrc b/.pylintrc
new file mode 100644
index 0..0db42d646
--- /dev/null
+++ b/.pylintrc
@@ -0,0 +1,577 @@
+[MASTER]
+
+# Specify a configuration file.
+#rcfile=
+
+# Add files or directories to the blacklist. They should be base names, not
+# paths.
+ignore=
+
+# Pickle collected data for later comparisons.
+persistent=no
+
+# List of plugins (as comma separated values of python modules names) to load,
+# usually to register additional checkers.
+load-plugins=
+
+# Use multiple processes to speed up Pylint.
+# DO NOT CHANGE THIS VALUES >1 HIDE RESULTS!
+jobs=1
+
+# Allow loading of arbitrary C extensions. Extensions are imported into the
+# active Python interpreter and may run arbitrary code.
+unsafe-load-any-extension=no
+
+# A comma-separated list of package or module names from where C extensions may
+# be loaded. Extensions are loading into the active Python interpreter and may
+# run arbitrary code
+extension-pkg-whitelist=
+
+# Allow optimization of some AST trees. This will activate a peephole AST
+# optimizer, which will apply various small optimizations. For instance, it can
+# be used to obtain the result of joining multiple strings with the addition
+# operator. Joining a lot of strings can lead to a maximum recursion error in
+# Pylint and this flag can prevent that. It has one side effect, the resulting
+# AST will be different than the one from reality.
+optimize-ast=no
+
+
+[MESSAGES CONTROL]
+
+# Only show warnings with the listed confidence levels. Leave empty to show
+# all. Valid levels: HIGH, INFERENCE, INFERENCE_FAILURE, UNDEFINED
+confidence=
+
+# Enable the message, report, category or checker with the given id(s). You can
+# either give multiple identifier separated by comma (,) or put this option
+# multiple time. See also the "--disable" option for examples.
+disable=all
+
+enable=import-error,
+   import-self,
+   reimported,
+   wildcard-import,
+   misplaced-future,
+   relative-import,
+   deprecated-module,
+   unpacking-non-sequence,
+   invalid-all-object,
+   undefined-all-variable,
+   used-before-assignment,
+   cell-var-from-loop,
+   global-variable-undefined,
+   redefined-builtin,
+   redefine-in-handler,
+   unused-import,
+   unused-wildcard-import,
+   global-variable-not-assigned,
+   undefined-loop-variable,
+   global-statement,
+   global-at-module-level,
+   bad-open-mode,
+   redundant-unittest-assert,
+   boolean-datetime,
+   # Has common issues with our style due to
+   # https://github.com/PyCQA/pylint/issues/210
+   unused-variable
+
+# Things we'd like to enable someday:
+# redefined-outer-name (requires a bunch of work to clean up our code first)
+# undefined-variable (re-enable when pylint fixes 
https://github.com/PyCQA/pylint/issues/760)
+# no-name-in-module (giving us spurious warnings 
https://github.com/PyCQA/pylint/issues/73)
+# unused-argument (need to clean up or code a lot, e.g. prefix unused_?)
+
+# Things we'd like to try.
+# Procedure:
+# 1. Enable a bunch.
+# 2. See if there's spurious ones; if so disable.
+# 3. Record above.
+# 4. Remove from this list.
+   # deprecated-method,
+   # anomalous-unicode-escape-in-string,
+   # anomalous-backslash-in-string,
+   # not-in-loop,
+   # function-redefined,
+   # continue-in-finally,
+   # abstract-class-instantiated,
+   # star-needs-assignment-target,
+   # duplicate-argument-name,
+   # return-in-init,
+   # too-many-star-expressions,
+   # nonlocal-and-global,
+   # return-outside-function,
+   # return-arg-in-generator,
+   # invalid-star-assignment-target,
+   # bad-reversed-sequence,
+   # nonexistent-operator,
+   # yield-outside-function,
+   # init-is-generator,
+   # nonlocal-without-binding,
+   # lost-exception,
+   # assert-on-tuple,
+   # dangerous-default-value,
+   # duplicate-key,
+   # useless-else-on-loop,
+   # expression-not-assigned,
+   # confusing-with-statement,
+   # unnecessary-lambda,
+   # pointless-statement,
+   # pointless-string-statement,
+   # unnecessary-pass,
+   # unreachable,
+   # eval-used,
+   # exec-used,
+   # bad-builtin,
+   # using-constant-test,
+   # deprecated-lambda,
+   # bad-super-call,
+   # 

[GSoC][PATCH 2/2] git-ci: update .pylintrc to ignore current warning

[Viet Hung Tran]

pylint's check for unused variables, global statements,
redefined builtins, undefined loop variables, and unused imports are
disabled.

The current configuration allows git-p4.py to pass the check in Travis CI
in my forked repository.

Here is the link for the successful built: 
https://travis-ci.org/VietHTran/git/builds/353155158 

Signed-off-by: Viet Hung Tran 
---
 .pylintrc | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/.pylintrc b/.pylintrc
index 0db42d646..09a0019d6 100644
--- a/.pylintrc
+++ b/.pylintrc
@@ -60,20 +60,13 @@ enable=import-error,
used-before-assignment,
cell-var-from-loop,
global-variable-undefined,
-   redefined-builtin,
redefine-in-handler,
-   unused-import,
unused-wildcard-import,
global-variable-not-assigned,
-   undefined-loop-variable,
-   global-statement,
global-at-module-level,
bad-open-mode,
redundant-unittest-assert,
boolean-datetime,
-   # Has common issues with our style due to
-   # https://github.com/PyCQA/pylint/issues/210
-   unused-variable
 
 # Things we'd like to enable someday:
 # redefined-outer-name (requires a bunch of work to clean up our code first)
-- 
2.16.2.440.gc6284da

Re: [PATCH v5 04/13] csum-file: add CSUM_KEEP_OPEN flag

[Derrick Stolee]

On 3/13/2018 5:42 PM, Junio C Hamano wrote:

Derrick Stolee  writes:


On 2/26/2018 9:32 PM, Derrick Stolee wrote:

This patch is new to the series due to the interactions with the lockfile API
and the hashfile API. I need to ensure the hashfile writes the hash value at
the end of the file, but keep the file descriptor open so the lock is valid.

I welcome any susggestions to this patch or to the way I use it in the commit
that follows.

-- >8 --

I haven't gotten any feedback on this step of the patch. Could someone
take a look and let me know what you think?


Let me just say that I appreciate the level of detail you provided in 
answering this question. The discussion below is very illuminating.



Let's follow the commit-graph writing codepath to see what happens:

fd = hold_lock_file_for_update(, graph_name, 0);
...
f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);

The caller creates a lockfile, and then wraps its file descriptor in
a hashfile.

hashwrite_be32(f, GRAPH_SIGNATURE);
...

Then it goes on writing to the hashfile, growing the lockfile.

 ...
write_graph_chunk_large_edges(f, commits.list, commits.nr);

close_commit_graph();

And after writing all data out (oh by the way, why aren't we passing
commit_graph instance around and instead relying on a file-scope
static global?)...


Yeah, we should remove the global dependence. Is this a blocker for the 
series?


After this lands, I plan to add a one-commit patch that moves all global 
"commit_graph" state to the_repository in the spirit of Stefan's 
patches. For now, this is equivalent to "close_all_packs()".



hashclose(f, final_hash, CSUM_CLOSE | CSUM_FSYNC | CSUM_KEEP_OPEN);

We ask for the final hash value to be written to the file (and also
returned to us---although you do not seem to use that at all).  See
a comment on this, though, at the end.


I'm not using the final_hash anymore, since we are using the 
"$GIT_DIR/objects/info/commit-graph" filename always. If we make the 
file incremental, then we can use "final_hash" to name the split files. 
For now, I'll remove final_hash in favor of NULL.



commit_lock_file();

And then, we put the lockfile to its final place, while closing its
file descriptor.

The overall API sounds sensible, from the above.

However.

The function whose name is hashclose() that takes a flag word whose
possible bit value includes "Please close this thing" feels strange
enough (does it mean the hashclose() function does not close it if
CSUM_CLOSE is not given?), but adding another to the mix that lets
us say "Please close this (with or without FSYNC), oh by the way
please leave it open" feels a bit borderline to insanity.

I _think_ the word "close" in the name hashclose() is about closing
the (virtual) stream for the hashing that is overlayed on top of the
underlying file descriptor, and being able to choose between closing
and not closing the underlying file descriptor when "closing" the
hashing layer sort of makes sense.  So I won't complain too much
about hashclose() that takes optional CSUM_CLOSE flag.


I agree this "close" word is incorrect. We really want 
"finalize_hashfile()" which may include closing the file.



But then what does it mean to give KEEP_OPEN and CLOSE together?


This should have been a red flag that something was wrong.


The new caller (which is the only one that wants the nominally
nonsensical CLOSE|KEEP_OPEN combination, which is shown above) wants
the final checksum of the data sent over the (virtual) stream
computed and written, and the file descriptor fsync'ed, but the file
descriptor kept open.  As we _DO_ want to keep the verbs in flags
CSUM_CLOSE and CSUM_FSYNC to be about the underlying file
descriptor, I think your new code for KEEP_OPEN that is inside the
if() block that is for CSUM_CLOSE is an ugly hack, and your asking
for improvements is very much appreciated.

Let's step back and see what different behaviours the existing code
wants to support before your patch:

 - hashclose() is always about finializing the hash computation
   over the data sent through the struct hashfile (i.e. the
   virtual stream opened by hashfd()).  The optional *result can
   be used to receive this hash value, even when the caller does
   not want to write that hash value to the output stream.

 - when CSUM_CLOSE is given, however, the hash value is written
   out as the trailing record to the output stream and the stream
   is closed.  CSUM_FSYNC can instead be used to ensure that the
   data hits the disk platter when the output stream is closed.

 - when CSUM_CLOSE nor CSUM_FSYNC is not given, hash value is not
   written to the output stream (the caller takes responsibility
   of using *result), and the output stream is left open.

I think the first mistake in the existing code is to associate
"close the underlying stream" and "write 

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Igor Djordjevic]

Hi Sergey,

On 13/03/2018 17:10, Sergey Organov wrote:
> 
> > Hi Sergey, I've been following this discussion from the sidelines,
> > though I haven't had time to study all the posts in this thread in
> > detail. I wonder if it would be helpful to think of rebasing a merge as
> > merging the changes in the parents due to the rebase back into the
> > original merge. So for a merge M with parents A B C that are rebased to
> > A' B' C' the rebased merge M' would be constructed by (ignoring shell
> > quoting issues)
> >
> > git checkout --detach M
> > git merge-recursive A -- M A'
> > tree=$(git write-tree)
> > git merge-recursive B -- $tree B'
> > tree=$(git write-tree)
> > git merge-recursive C -- $tree C'
> > tree=$(git write-tree)
> > M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB' -pC')
> 
> I wonder if it's OK to exchange the order of heads in the first merge
> (also dropped C for brevity):

It should be, being "left" or "right" hand side ("theirs" or "ours") 
of the three-way merge shouldn`t matter, they`re still both equally 
compared to the merge-base.

> git checkout --detach A'
> git merge-recursive A -- A' M
> tree=$(git write-tree)
> git merge-recursive B -- $tree B'
> tree=$(git write-tree)
> M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB')
> 
> If so, don't the first 2 lines now read: "rebase (first parent of) M on
> top of A'"?

Hmm, lol, yes...? :) So basically, this:

(1) git checkout --detach M
git merge-recursive A -- M A'
tree=$(git write-tree)
...

... is equivalent to this:

(2) git checkout --detach A'
git merge-recursive A -- A' M
tree=$(git write-tree)
...

..., being equivalent to this:

(3) git checkout --detach A'
git cherry-pick -m 1 M
tree=$(git write-tree)
...

..., where in all three cases that `$tree` is equivalent to U1' we 
discussed about so much already :)

I tested it like this as well, slightly modifying previously sent out 
script (like this one[1]), and it still seems to be working ;) Nice!

> If so, then it could be implemented so that it reduces back to regular
> rebase of non-merges when applied to a single-parent commit, similar to
> the method in the RFC, striking out one of advantages of the RFC.

I guess so, but I think it now boils down only to what one finds 
easier to reason about even more.

I`m just glad we got to U1' from this perspective as well, hopefully 
adding even more faith in the overall concept, being beaten from both 
ends and dropping out to be the same (minus minor implementation details).

Regards, Buga

[1] https://public-inbox.org/git/872944c4-ca97-9f55-a424-86d1e3299...@gmail.com/

[GSoC][PATCH] git-ci: use pylint to analyze the git-p4 code

[Viet Hung Tran]

Hello Mr. Diamand,

I see. Initially, I also thought of using .pylintrc but I don’t know 
which error should be ignored and which should be fixed so  I decided 
to adhere 100% to the convention and fix each type of errors in 
different patches.

Thank you for your help,

Viet

Signed-off-by: Viet Hung Tran 

Re: [PATCH] Documentation/githooks: Clarify the behavior of post-checkout hook

[Jonathan Nieder]

Hi,

Magne Land wrote:

> From: Magne Land 
>
> This can happen when using 'git rebase -i’:
> could not detach HEAD
>
> Based on discovering this Stack Overflow discussion:
> https://stackoverflow.com/questions/25561485/git-rebase-i-with-squash-cannot-detach-head
> ---
>  Documentation/githooks.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Thanks for investigating and writing this.

May we forge your sign-off?  See Documentation/SubmittingPatches
section [[sign-off] 'Certify your work' for more about what this
means.

The above leaves one question unanswered: is this the *right* behavior
for "git checkout" to have?  I.e. is it useful for "git checkout" to
fail when the post-checkout hook fails, or would it be better for it
to e.g. simply print a message and exit with status 0?

Not a rhetorical question: I'm asking because I don't know the answer.
What do you think?

Thanks,
Jonathan

> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -166,7 +166,9 @@ worktree.  The hook is given three parameters: the ref of 
> the previous HEAD,
>  the ref of the new HEAD (which may or may not have changed), and a flag
>  indicating whether the checkout was a branch checkout (changing branches,
>  flag=1) or a file checkout (retrieving a file from the index, flag=0).
> -This hook cannot affect the outcome of 'git checkout'.
> +
> +If this hook exits with a non-zero status, 'git checkout' will exit with the
> +same status.
>  
>  It is also run after 'git clone', unless the --no-checkout (-n) option is
>  used. The first parameter given to the hook is the null-ref, the second the

Re: allow "~" to be present in a tag name

[Jonathan Nieder]

Hi Michal,

Michal Novotny wrote:

> currently, if I try to create a tag that has tilde "~"  in name, an
> error is raised. E.g.
>
> $ git tag rpkg-util-1.4~rc1
> fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.

As Ævar mentioned, this is disallowed to prevent a collision with
Git's revision specifying syntax.

While I'm sympathetic to wanting the tag name to match the version
number used by the package manager, the line has to be drawn
somewhere.  "git help check-ref-format" describes the current
namespace:

Git imposes the following rules on how references are named:

1. They can include slash / for hierarchical (directory)
   grouping, but no slash-separated component can begin with a
   dot . or end with the sequence .lock.

2. They must contain at least one /. This enforces the
   presence of a category like heads/, tags/ etc. but the
   actual names are not restricted. If the --allow-onelevel
   option is used, this rule is waived.

3. They cannot have two consecutive dots .. anywhere.

4. They cannot have ASCII control characters (i.e. bytes whose
   values are lower than \040, or \177 DEL), space, tilde ~,
   caret ^, or colon : anywhere.

5. They cannot have question-mark ?, asterisk *, or open
   bracket [ anywhere. See the --refspec-pattern option below
   for an exception to this rule.

6. They cannot begin or end with a slash / or contain multiple
   consecutive slashes (see the --normalize option below for
   an exception to this rule)

7. They cannot end with a dot ..

8. They cannot contain a sequence @{.

9. They cannot be the single character @.

   10. They cannot contain a \.

If anything, I suspect the current namespace is too wide.  For
example, it has issues with Unicode normalization in filenames on some
platforms, and it allows some potentially problematic characters like
` and |.

So my first instinct is to recommend that you apply some mapping
between your packager manager's version syntax and Git's tag syntax
--- e.g. using -rc1 as Ævar suggested, or using urlencoding %7Erc1 as
you hinted.

That isn't to say that this would be impossible to loosen.  But my
worry is that it's hard to decide where to draw the line: there are a
number of sets of names that might want to be valid tags, and it is
hard to say which are worth the complexity of expanding the set of
valid ref names.  That's why my first reaction is to look around for
another way to accomplish your goal.

Thanks,
Jonathan

Re: Opinions on changing add/add conflict resolution?

[Junio C Hamano]

On Tue, Mar 13, 2018 at 4:14 PM, Elijah Newren  wrote:
>
> Someone in the future might hate us if they use conflictstyle=diff3,
> and have a recursive merge,

FWIW I already always use diff3 style and see the nested markers,
and I already hate it, so you are no worse off ;-)

REPLY BACK PLEASE

[Doris Markkoh]

-- 

Hello dear,
How are you doing today? I got your contact from a directory and
picked interest to communicate you by faith, though we have not met
before, I believe we can achieve something together. My husband died
few years ago after battling with brain cancer, before his death, he
deposited ($10.5 Million) in one of the financial institution. He
wanted to establish coco processing factory and also real estate
business.

After the death of my husband, I have been receiving all manner of
life threats from the family members, now the pressure is getting more
severe I decided to leave this country. Please can you partner with me
and receive the fund in your account while I come over there with my
son for investment. If you agree, get back for more details and what
will be your commission for the assistance. Call me please:
+22967650091
I wait for your reply,
Doris Markkoh

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Tue, Mar 13, 2018 at 3:56 PM, Jonathan Nieder  wrote:
> Elijah Newren wrote:
>
>> However, my question here about what to write to the working tree for
>> a rename/rename(2to1) conflict in one particular corner case still
>> remains.  Should a two-way merge be performed even if it may result in
>> nested sets of conflict markers, or is that a sufficiently bad outcome
>> for the user that it's the one case we do want to write colliding
>> files out to different temporary paths?
>
> Nested conflict markers only happen in the conflictstyle=diff3 case, I
> would think.

Currently, yes.  To be clear, though, this change would make it
possible even when there is no recursive merge being done and when
conflictstyle=merge.

> merge-recursive writes them already.  I've often wished that it would
> use a union merge strategy when building the common ancestor to avoid
> the nested conflicts that rerere doesn't understand.  But anyway,
> that's an orthogonal issue: in the rename/rename context, it should be
> fine to write nested conflict markers since that's consistent with
> what merge-recursive already does.

Cool, sounds like we're now all on the same page.

Someone in the future might hate us if they use conflictstyle=diff3,
and have a recursive merge, and have a rename/rename(2to1) conflict in
the virtual merge base with nested conflicts, and that resulting file
is also involved in a separate rename/rename(2to1) conflict in the
outer merge that has its own nested conflicts; we'd have up to four
levels of nested conflicts.  But for now, I'm going to write that off
as a crazy thought (or dismiss it as nigh impossible to reason about
regardless of what we did to help them) and just proceed ahead.  :-)

RE: recent glob expansion breakage on Windows?

[Carsey, Jaben]

I do not remember what version I was using before.  I am suddenly wondering if 
I previously sent single patches instead of using wildcard (which works fine).  
The only person I have found doing patch series here on windows uses the 
directory method (put patch files there, list directory name on send-email 
command line).

I have updated to the 2.16.2 version and I see the same issues.

-Jaben

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Friday, March 09, 2018 1:42 AM
> To: Jonathan Nieder 
> Cc: public git mailing list ; Carsey, Jaben
> ; git-for-wind...@googlegroups.com
> Subject: Re: recent glob expansion breakage on Windows?
> Importance: High
> 
> On 03/08/18 23:03, Jonathan Nieder wrote:
> > +git-for-windows
> > Hi,
> >
> > Laszlo Ersek wrote:
> >
> >> Jaben reports that git-send-email is suddenly failing to expand the
> >> "*.patch" glob for him, at the Windows CMD prompt:
> >>
> >> -
> >> E:\...>git send-email --suppress-cc=author --suppress-cc=self --suppress-
> cc=cc --suppress-cc=sob --dry-run *.patch
> >>
> >> No patch files specified!
> >> -
> >>
> >> Whereas, moving the same patch files to another subdir, and then passing
> >> the subdir to git-send-email, works fine.
> >>
> >> I seem to have found some $^O based perl code in the git tree that
> >> expands the glob manually (in place of the shell) on Windows -- however,
> >> that code looks ancient ^W very stable, and doesn't seem to explain the
> >> sudden breakage.
> >>
> >> Is it possible that a separate perl update on Jaben's Windows box is
> >> causing this? Or does the breakage look consistent with a recent git
> change?
> >>
> >> Has anyone else reported something similar recently?
> >
> > This reminds me of https://github.com/git-for-windows/git/issues/339.
> > There, Johannes Schindelin writes (about a different command):
> >
> > | This is expected because neither PowerShell nor cmd.exe nor git.exe
> > | expand wildcards. Those examples you found were written with a shell
> > | in mind, and the shell expands wildcards (hence Git does not think
> > | it needs to).
> >
> > That may or may not also apply to send-email.
> 
> Thank you for the reference -- I can't say whether closing issue #339 as
> WONTFIX was justified or not, but it certainly seems inconsistent with
> Jaben's earlier experience (to my understanding), i.e. that git did
> expand the glob.
> 
> > In what version did it work?
> 
> Jaben, can you please answer that? (One version in which it is broken is
> 2.14.1.windows.1.) Can you perhaps ask your teammates about their
> git/windows versions (assuming the *.patch glob is expanded correctly
> for them)?
> 
> Thank you, Jonathan,
> Laszlo

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Tue, Mar 13, 2018 at 3:52 PM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>> However, my question here about what to write to the working tree for
>> a rename/rename(2to1) conflict in one particular corner case still
>> remains.
>
> Hmph, is it a bad idea to model this after what recursive merge
> strategy does?  I think what is written out from that codepath to
> the working tree has the nested conflict markers (with a bit of
> tweak to the marker length, IIRC) in it.

Oh, that's cool; I didn't know that.  It looks like that was
introduced in commit d694a17986 ("ll-merge: use a longer conflict
marker for internal merge", 2016-04-14).  That seems like a good idea;
I'll go with that.  Thanks.

Re: Opinions on changing add/add conflict resolution?

[Jonathan Nieder]

Hi,

Elijah Newren wrote:

> However, my question here about what to write to the working tree for
> a rename/rename(2to1) conflict in one particular corner case still
> remains.  Should a two-way merge be performed even if it may result in
> nested sets of conflict markers, or is that a sufficiently bad outcome
> for the user that it's the one case we do want to write colliding
> files out to different temporary paths?

Nested conflict markers only happen in the conflictstyle=diff3 case, I
would think.

merge-recursive writes them already.  I've often wished that it would
use a union merge strategy when building the common ancestor to avoid
the nested conflicts that rerere doesn't understand.  But anyway,
that's an orthogonal issue: in the rename/rename context, it should be
fine to write nested conflict markers since that's consistent with
what merge-recursive already does.

Thanks,
Jonathan

Re: Opinions on changing add/add conflict resolution?

[Junio C Hamano]

Elijah Newren  writes:

> However, my question here about what to write to the working tree for
> a rename/rename(2to1) conflict in one particular corner case still
> remains.

Hmph, is it a bad idea to model this after what recursive merge
strategy does?  I think what is written out from that codepath to
the working tree has the nested conflict markers (with a bit of
tweak to the marker length, IIRC) in it.

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Tue, Mar 13, 2018 at 3:26 PM, Junio C Hamano  wrote:
> Elijah Newren  writes:
>
>> Cool, thanks for the context.  I'm happy to go down this path, but
>> there is one question I'd like your opinion on: what if the
>> intermediate content merges have conflicts themselves?  If that
>> question isn't clear, let me be more precise...
>
> I think you answered this yourself after (re)discovering the virtual
> ancestor merge in the recursive strategy, and no longer need my
> input here ;-)

The question about what to put into the index was another issue, and
it's good to hear that you seem to approve of my logic on that one.
Thanks.  :-)

However, my question here about what to write to the working tree for
a rename/rename(2to1) conflict in one particular corner case still
remains.  Should a two-way merge be performed even if it may result in
nested sets of conflict markers, or is that a sufficiently bad outcome
for the user that it's the one case we do want to write colliding
files out to different temporary paths?

你好！你好吗？

[Hannah Justin]

Re: Opinions on changing add/add conflict resolution?

[Junio C Hamano]

Elijah Newren  writes:

> Cool, thanks for the context.  I'm happy to go down this path, but
> there is one question I'd like your opinion on: what if the
> intermediate content merges have conflicts themselves?  If that
> question isn't clear, let me be more precise...

I think you answered this yourself after (re)discovering the virtual
ancestor merge in the recursive strategy, and no longer need my
input here ;-)

Re: [PATCH v2] git{,-blame}.el: remove old bitrotting Emacs code

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> I sent a v3 a few days ago which addressed this, it's at
> 20180310184545.16950-1-ava...@gmail.com

Thanks, I did look at that version but did not act on it immediately
and then forgot about it.

Queued.

Re: Fwd: Opinions on changing add/add conflict resolution?

[Junio C Hamano]

Elijah Newren  writes:

> As currently implemented, yes.  However, I was more concerned the idea
> of handling files differently based on whether or not they were
> similar, rather than on what the precise definition of "similar" is
> for this context.
>
> As far as the definition of similarity goes, estimate_similarity() is
> currently used by rename detection to compare files recorded at
> different pathnames.  By contrast, in this context, we are comparing
> two files which were recorded with the same pathname.  That suggests
> the heuristic could be a bit different and use more than just
> estimate_similarity().  (e.g. "We consider these files similar IF more
> than 50% of the lines match OR both files are less than 2K.")

Yeah, I think there is similar difference between similarity score
that is used by diffcore-rename and dissimilarity score that is used
by diffcore-break exactly for that reason.  If you start from a
100-line original file and grow it to 100-line one by adding 900
lines, as long as you kept the original 100 lines, it is easier to
view the change within the same path as a continued development,
instead of saying they are so dissimilar.

In any case, I think the way the stage #2 and stage #3 (i.e. ours
and theirs) contents are externalized during a conflicted mergy
operation should be consistent across edit/edit and add/add
conflict, so if we are adding a new way to write out extra temporary
files out of these higher stage index entries, it should be made
applicable not only to add/add conflict (i.e. there shounld't be a
code that says "oh, let's see, this lacks stage #1, so do this
different thing").

Personally, I think it is best to leave it all outside of the core
and make "git mergetool" to be responsible for the job of
externalizing higher stage index entries to temporary working tree
files.  They already need to do so in order to work with external
tools that do not read directly from our index file anyway, no?

Re: git-log bug: --grep and -number args are at odds

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Alexander Mills jotted:

> $ git log -50   ###  I get 50 results
>
> $ git log -50 --grep="*"   ### I get a lot more than 50...
>
> shouldn't `-50` minimize the results length to 50 or fewer?

On what git version? On my 2.16.2.804.g6dcf76e118 on git.git this works
as expected:

$ git -c grep.patternType=basic log --oneline -50 --grep="*" |wc -l
50
$ git -c grep.patternType=basic log --oneline -50 | wc -l
50

Re: [PATCH v4 20/35] upload-pack: introduce fetch server command

[Brandon Williams]

On 03/13, Jonathan Tan wrote:
> On Wed, 28 Feb 2018 15:22:37 -0800
> Brandon Williams  wrote:
> 
> > +output = *section
> > +section = (acknowledgments | packfile)
> > + (flush-pkt | delim-pkt)
> > +
> > +acknowledgments = PKT-LINE("acknowledgments" LF)
> > + (nak | *ack)
> > + (ready)
> > +ready = PKT-LINE("ready" LF)
> > +nak = PKT-LINE("NAK" LF)
> > +ack = PKT-LINE("ACK" SP obj-id LF)
> > +
> > +packfile = PKT-LINE("packfile" LF)
> > +  [PACKFILE]
> 
> I should have noticed this earlier, but "PACKFILE" is not defined anywhere -
> it's probably better written as:
> 
> *PKT-LINE(%x01-03 *%x00-ff)"
> 
> or something like that.

I'll document it as you described.

> 
> > +acknowledgments section
> > +   * Always begins with the section header "acknowledgments"
> > +
> > +   * The server will respond with "NAK" if none of the object ids sent
> > + as have lines were common.
> > +
> > +   * The server will respond with "ACK obj-id" for all of the
> > + object ids sent as have lines which are common.
> > +
> > +   * A response cannot have both "ACK" lines as well as a "NAK"
> > + line.
> > +
> > +   * The server will respond with a "ready" line indicating that
> > + the server has found an acceptable common base and is ready to
> > + make and send a packfile (which will be found in the packfile
> > + section of the same response)
> > +
> > +   * If the client determines that it is finished with negotiations
> > + by sending a "done" line, the acknowledgments sections MUST be
> > + omitted from the server's response.
> > +
> > +   * If the server has found a suitable cut point and has decided
> > + to send a "ready" line, then the server can decide to (as an
> > + optimization) omit any "ACK" lines it would have sent during
> > + its response.  This is because the server will have already
> > + determined the objects it plans to send to the client and no
> > + further negotiation is needed.
> > +
> > +
> > +packfile section
> > +   * Always begins with the section header "packfile"
> > +
> > +   * The transmission of the packfile begins immediately after the
> > + section header
> > +
> > +   * The data transfer of the packfile is always multiplexed, using
> > + the same semantics of the 'side-band-64k' capability from
> > + protocol version 1.  This means that each packet, during the
> > + packfile data stream, is made up of a leading 4-byte pkt-line
> > + length (typical of the pkt-line format), followed by a 1-byte
> > + stream code, followed by the actual data.
> > +
> > + The stream code can be one of:
> > +   1 - pack data
> > +   2 - progress messages
> > +   3 - fatal error message just before stream aborts
> > +
> > +   * This section is only included if the client has sent 'want'
> > + lines in its request and either requested that no more
> > + negotiation be done by sending 'done' or if the server has
> > + decided it has found a sufficient cut point to produce a
> > + packfile.
> 
> For both the sections, I think that the conditions for
> inclusion/non-inclusion ("This section is only included if...") should
> be the first point.
> 
> > +static void upload_pack_data_init(struct upload_pack_data *data)
> > +{
> > +   struct object_array wants = OBJECT_ARRAY_INIT;
> > +   struct oid_array haves = OID_ARRAY_INIT;
> > +
> > +   memset(data, 0, sizeof(*data));
> > +   data->wants = wants;
> > +   data->haves = haves;
> > +}
> 
> Any reason to use a initializer function instead of a static literal?

Its much cleaner and easier to read than it was when i was using an
initializer.

-- 
Brandon Williams

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Junio C Hamano]

Martin Ågren  writes:

> in order to decide whether we should read from stdin. (So yes, after
> this patch, we will still silently ignore stdin for confused usage such
> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
> not crash.)

$ git log -p | git shortlog Documentation/

is also a nonsense request.  When outside a repository, i.e.

$ git -C $path_to_repo | git shortlog Documentation/

is not giving any revisions, so the error message should not say "no
revisions can be given"---a nitpicky bug reporter would say "I gave
no revisions, why are you complaining to me?"

Re: [PATCH v5 04/13] csum-file: add CSUM_KEEP_OPEN flag

[Junio C Hamano]

Derrick Stolee  writes:

> On 2/26/2018 9:32 PM, Derrick Stolee wrote:
>> This patch is new to the series due to the interactions with the lockfile API
>> and the hashfile API. I need to ensure the hashfile writes the hash value at
>> the end of the file, but keep the file descriptor open so the lock is valid.
>>
>> I welcome any susggestions to this patch or to the way I use it in the commit
>> that follows.
>>
>> -- >8 --
>
> I haven't gotten any feedback on this step of the patch. Could someone
> take a look and let me know what you think?

Let's follow the commit-graph writing codepath to see what happens:

fd = hold_lock_file_for_update(, graph_name, 0);
...
f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);

The caller creates a lockfile, and then wraps its file descriptor in
a hashfile.

hashwrite_be32(f, GRAPH_SIGNATURE);
...

Then it goes on writing to the hashfile, growing the lockfile.

...
write_graph_chunk_large_edges(f, commits.list, commits.nr);

close_commit_graph();

And after writing all data out (oh by the way, why aren't we passing
commit_graph instance around and instead relying on a file-scope
static global?)...

hashclose(f, final_hash, CSUM_CLOSE | CSUM_FSYNC | CSUM_KEEP_OPEN);

We ask for the final hash value to be written to the file (and also
returned to us---although you do not seem to use that at all).  See
a comment on this, though, at the end.

commit_lock_file();

And then, we put the lockfile to its final place, while closing its
file descriptor.

The overall API sounds sensible, from the above.

However.

The function whose name is hashclose() that takes a flag word whose
possible bit value includes "Please close this thing" feels strange
enough (does it mean the hashclose() function does not close it if
CSUM_CLOSE is not given?), but adding another to the mix that lets
us say "Please close this (with or without FSYNC), oh by the way
please leave it open" feels a bit borderline to insanity.

I _think_ the word "close" in the name hashclose() is about closing
the (virtual) stream for the hashing that is overlayed on top of the
underlying file descriptor, and being able to choose between closing
and not closing the underlying file descriptor when "closing" the
hashing layer sort of makes sense.  So I won't complain too much
about hashclose() that takes optional CSUM_CLOSE flag.

But then what does it mean to give KEEP_OPEN and CLOSE together?

The new caller (which is the only one that wants the nominally
nonsensical CLOSE|KEEP_OPEN combination, which is shown above) wants
the final checksum of the data sent over the (virtual) stream
computed and written, and the file descriptor fsync'ed, but the file
descriptor kept open.  As we _DO_ want to keep the verbs in flags
CSUM_CLOSE and CSUM_FSYNC to be about the underlying file
descriptor, I think your new code for KEEP_OPEN that is inside the
if() block that is for CSUM_CLOSE is an ugly hack, and your asking
for improvements is very much appreciated.

Let's step back and see what different behaviours the existing code
wants to support before your patch:

- hashclose() is always about finializing the hash computation
  over the data sent through the struct hashfile (i.e. the
  virtual stream opened by hashfd()).  The optional *result can
  be used to receive this hash value, even when the caller does
  not want to write that hash value to the output stream.

- when CSUM_CLOSE is given, however, the hash value is written
  out as the trailing record to the output stream and the stream
  is closed.  CSUM_FSYNC can instead be used to ensure that the
  data hits the disk platter when the output stream is closed.

- when CSUM_CLOSE nor CSUM_FSYNC is not given, hash value is not
  written to the output stream (the caller takes responsibility
  of using *result), and the output stream is left open.

I think the first mistake in the existing code is to associate
"close the underlying stream" and "write the hash out to the
underlying stream" more closely than it should.  It should be
possible to "close the underlying steam" without first writing the
hash out to the underlying stream", and vice versa.

IOW, I think

hashclose() {
hashflush();
the_hash_algo->final_fn();
if (result) 
hashcpy(result, f->buffer);
+   if (flags & CSUM_HASH_IN_STREAM)
+   flush(f, f->buffer, the_hash_algo->rawsz);
+   if (flags & CSUM_FSYNC)
+   fsync_or_die();
if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
-   flush();
-   if (flags & CSUM_FSYNC)
-   fsync_or_die();
if (close(f->fd))
die_errno();

git-log bug: --grep and -number args are at odds

[Alexander Mills]

$ git log -50   ###  I get 50 results

$ git log -50 --grep="*"   ### I get a lot more than 50...

shouldn't `-50` minimize the results length to 50 or fewer?

-alex

-- 
Alexander D. Mills
! New cell phone number: (415)766-8243
alexander.d.mi...@gmail.com

www.linkedin.com/pub/alexander-mills/b/7a5/418/

Re: [PATCH v4 12/35] serve: introduce git-serve

[Brandon Williams]

On 03/02, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > + Capabilities
> > +~~
> > +
> > +There are two different types of capabilities: normal capabilities,
> > +which can be used to to convey information or alter the behavior of a
> > +request, and commands, which are the core actions that a client wants to
> > +perform (fetch, push, etc).
> > +
> > +All commands must only last a single round and be stateless from the
> > +perspective of the server side.  All state MUST be retained and managed
> > +by the client process.  This permits simple round-robin load-balancing
> > +on the server side, without needing to worry about state management.
> > +
> > +Clients MUST NOT require state management on the server side in order to
> > +function correctly.
> 
> This somehow feels a bit too HTTP centric worldview that potentially
> may penalize those who do not mind stateful services.

It's meant to be that way so that we don't run into the same issue we
have with the current HTTP transport.  Though I've decided to loosen
this slightly by making protocol v2 stateless by default unless a
capability is advertised and requested by the client indicating that
state can be maintained by the server.  That leaves the door open for
adding state later for transports which have full-duplex connections
while still requiring that stateless is designed first.  I'm kind of
hoping we never need to add state to the protocol because hopefully we
can figure out how to improve negotiation as a whole.

> 
> > + agent
> > +---
> > +
> > +The server can advertise the `agent` capability with a value `X` (in the
> > +form `agent=X`) to notify the client that the server is running version
> > +`X`.  The client may optionally send its own agent string by including
> > +the `agent` capability with a value `Y` (in the form `agent=Y`) in its
> > +request to the server (but it MUST NOT do so if the server did not
> > +advertise the agent capability).
> 
> Are there different degrees of permissiveness between "The server
> CAN" and "The client MAY" above, or is the above paragraph merely
> being fuzzy?

I don't think so? I believe I ripped this from the existing description
of the agent capability from the current protocol.

-- 
Brandon Williams

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Jonathan Nieder]

Martin Ågren wrote:
> On 13 March 2018 at 20:56, Jonathan Nieder  wrote:
>> Martin Ågren wrote:

>>>   (So yes, after
>>> this patch, we will still silently ignore stdin for confused usage such
>>> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
>>> not crash.)
>>
>> I don't follow here.  Are you saying this command should notice that
>> there is input in stdin?  How would it notice?
>
> I have no idea how it would notice (portably!) and the gain seems
> minimal. I added this to keep the reader from wondering "but wait a
> minute, doesn't that mean that we fail to catch this bad usage if we're
> in a repo?". So my answer would be "yep, but it's not a huge problem".
> Of course, my attempt to pre-emptively answer a question only provoked
> another one. :-) I could phrase this better.

Ah, I think I see what I was missing.  Let me look again at the whole
paragraph in context:

[...]
>>> Disallow left-over arguments when run from outside a repo. Another
>>> approach would be to disallow them when reading from stdin. However, our
>>> logic is currently the other way round: we check the number of revisions
>>> in order to decide whether we should read from stdin. (So yes, after
>>> this patch, we will still silently ignore stdin for confused usage such
>>> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
>>> not crash.)

How about something like this?

Disallow left-over arguments when run from outside a repo.  This
way, such invalid usage is diagnosed correctly:

$ git shortlog v2.16.0..
error: [...]
[...]

while still permitting the piped form:

$ git -C ~/src/git log --pretty=short | git shortlog
A Large Angry SCM (15):
[...]

This cannot catch an incorrect usage that combines the piped and
 forms:

$ git log --pretty=short | git shortlog v2.16.0..
Alban Gruin (1)
[...]

but (1) the operating system does not provide a straightforward
way to detect that and (2) at least it doesn't crash.

That detail is mostly irrelevant to what the patch does, though.  I
wouldn't expect git to be able to diagnose that third variant anyway.
If we want to make the command less error-prone, I think a good path
forward would be to introduce an explicit --stdin option.  So I'd be
tempted to go with the short and sweet:

Disallow left-over arguments when run from outside a repo.

[...]
>>> + error(_("no revisions can be given when running "
>>> + "from outside a repository"));
>>> + usage_with_options(shortlog_usage, options);
>>> + }
>>> +
>>
>> The error message is
>>
>> error: no revisions can be given when running from outside a 
>> repository
>> usage: ...
>>
>> Do we need to dump usage here?  I wonder if a simple die() call would
>> be easier for the user to act on.
>
> I can see an argument for "dumping the usage makes the error harder than
> necessary to find". I mainly went for consistency. This ties into your
> other observations below: what little consistency do we have and in
> which direction do we want to push it...
[...]
> I think it would be a larger project to make these consistent. The one
> I'm adding here is at least consistent with the other one in this file.

Ah, thanks.  That makes sense.

>> Separate from that, I wonder if the error message can be made a little
>> shorter and clearer.  E.g.
>>
>> fatal: shortlog  can only be used inside a git repository
>
> Some grepping suggests we do not usually name the command ("shortlog
> ..."), perhaps to play well with aliasing, nor do we use "such "
> very often, but it does happen. Quoting and allowing for options might
> make this more correct, but perhaps less readable: "'shortlog [...]
> ' can only ...". Slightly better than what I had, "revisions can
> only be given inside a git repository" would avoid some negating.

Sounds good to me.

Thanks,
Jonathan

Re: [PATCH v4 12/35] serve: introduce git-serve

[Brandon Williams]

On 03/02, Junio C Hamano wrote:
> Brandon Williams  writes:
> > +static int is_command(const char *key, struct protocol_capability 
> > **command)
> > +{
> > +   const char *out;
> > +
> > +   if (skip_prefix(key, "command=", )) {
> > +   struct protocol_capability *cmd = get_capability(out);
> > +
> > +   if (!cmd || !cmd->advertise(the_repository, NULL) || 
> > !cmd->command)
> > +   die("invalid command '%s'", out);
> > +   if (*command)
> > +   die("command already requested");
> 
> Shouldn't these two checks that lead to die the other way around?
> When they give us "command=frotz" and we already have *command, it
> would be an error whether we understand 'frotz' or not.
> 
> Who are the target audience of these "die"?  Are they meant to be
> communicated back to the other side of the connection, or are they
> only to be sent to the "server log"?
> 
> The latter one may want to say what two conflicting commands are in
> the log message, perhaps?

Yeah I'll switch the order of these checks as well as print out the two
commands requested for better logging.

> 
> > +   *command = cmd;
> 

-- 
Brandon Williams

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Brandon Williams]

On 03/02, Junio C Hamano wrote:
> Brandon Williams  writes:
> 
> > + ls-refs
> > +-
> > +
> > +`ls-refs` is the command used to request a reference advertisement in v2.
> > +Unlike the current reference advertisement, ls-refs takes in arguments
> > +which can be used to limit the refs sent from the server.
> 
> OK.
> 
> > +Additional features not supported in the base command will be advertised
> > +as the value of the command in the capability advertisement in the form
> > +of a space separated list of features, e.g.  "=
> > +".
> 
> Doesn't this explain the general convention that applies to any
> command, not just ls-refs command?  As a part of ls-refs section,
>  in the above explanation is always a constant "ls-refs",
> right?
> 
> It is a bit unclear how  in the above description are
> related to "arguments" in the following paragraph.  Do the server
> that can show symref and peeled tags and that can limit the output
> with ref-pattern advertise these three as supported features, i.e.
> 
>   ls-refs=symrefs peel ref-pattern
> 
> or something?  Would there a case where a "feature" does not
> correspond 1:1 to an argument to the command, and if so how would
> the server and the client negotiate use of such a feature?

I mention earlier in the document that the values of each capability are
to be defined by the capability itself, so I'm just documenting what the
value advertised means.  And a feature could mean a couple things and
doesn't necessarily mean it affects the arguments which can be provided,
and it definitely doesn't mean that each argument that can be provided
must be advertised as a feature.  If you look at the patch that
introduces shallow, the shallow feature adds lots of arguments that a
client can that use in its request.

> 
> > +ref-pattern 
> > +   When specified, only references matching one of the provided
> > +   patterns are displayed.  A pattern is either a valid refname
> > +   (e.g.  refs/heads/master), in which a ref must match the pattern
> > +   exactly, or a prefix of a ref followed by a single '*' wildcard
> > +   character (e.g. refs/heads/*), in which a ref must have a prefix
> > +   equal to the pattern up to the wildcard character.
> 
> I thought the recent concensus was left-anchored prefix match that
> honors /-directory boundary, i.e. no explicit asterisk and just
> saying "refs/heads" is enough to match "refs/heads" itself and
> "refs/heads/master" but not "refs/headscarf"?

I don't think there was a consensus at the time, but in the next
revision I'll have them be prefixes instead of containing wildcards.

-- 
Brandon Williams

Re: [PATCH v4 13/35] ls-refs: introduce ls-refs server command

[Brandon Williams]

On 03/05, Jeff King wrote:
> On Mon, Mar 05, 2018 at 10:21:55AM -0800, Brandon Williams wrote:
> 
> > > Hmm, so this would accept stuff like "refs/heads/*/foo" but quietly
> > > ignore the "/foo" part.
> > 
> > Yeah that's true...this should probably not do that.  Since
> > "refs/heads/*/foo" violates what the spec is, really this should error
> > out as an invalid pattern.
> 
> Yeah, that would be better, I think.
> 
> > > It also accepts "refs/h*" to get "refs/heads" and "refs/hello".  I think
> > > it's worth going for the most-restrictive thing to start with, since
> > > that enables a lot more server operations without worrying about
> > > breaking compatibility.
> > 
> > And just to clarify what do you see as being the most-restrictive case
> > of patterns that would should use?
> 
> I mean only accepting "*" at a "/" boundary (or just allowing a trailing
> slash to imply recursion, like "refs/heads/", or even just always
> assuming recursion to allow "refs/heads").

For simplicity I'll change ref-patterns to be ref-prefixes where
a ref must start_with() one of the provided ref-prefixes.  Clients won't
send '*'s either but can send everything upto but not including the '*'
as a prefix.

-- 
Brandon Williams

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Martin Ågren]

On 13 March 2018 at 20:56, Jonathan Nieder  wrote:
> Hi,
>
> Martin Ågren wrote:
>
>>   (So yes, after
>> this patch, we will still silently ignore stdin for confused usage such
>> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
>> not crash.)
>
> I don't follow here.  Are you saying this command should notice that
> there is input in stdin?  How would it notice?

I have no idea how it would notice (portably!) and the gain seems
minimal. I added this to keep the reader from wondering "but wait a
minute, doesn't that mean that we fail to catch this bad usage if we're
in a repo?". So my answer would be "yep, but it's not a huge problem".
Of course, my attempt to pre-emptively answer a question only provoked
another one. :-) I could phrase this better.

>> --- a/builtin/shortlog.c
>> +++ b/builtin/shortlog.c
>> @@ -293,6 +293,12 @@ int cmd_shortlog(int argc, const char **argv, const 
>> char *prefix)
>>  parse_done:
>>   argc = parse_options_end();
>>
>> + if (nongit && argc != 1) {
>
> Just curious: would argc ever be 0 here?  'argc <= 1' might be clearer.

Hmm, good point. It "shouldn't" be 0, but I guess it's better to be safe
than sorry. (We seem to have both constructs, in various places.)

>> + error(_("no revisions can be given when running "
>> + "from outside a repository"));
>> + usage_with_options(shortlog_usage, options);
>> + }
>> +
>
> The error message is
>
> error: no revisions can be given when running from outside a 
> repository
> usage: ...
>
> Do we need to dump usage here?  I wonder if a simple die() call would
> be easier for the user to act on.

I can see an argument for "dumping the usage makes the error harder than
necessary to find". I mainly went for consistency. This ties into your
other observations below: what little consistency do we have and in
which direction do we want to push it...

> Not about this patch: I was a little surprised to see 'error:' instead
> of 'usage:' or 'fatal:'.  It turns out git is pretty inconsistent
> about that: e.g. there is
>
> error(_("no remote specified"));
> usage_with_options(builtin_remote_setbranches_usage, options);
>
> Some other callers just use usage_with_options without describing the
> error.

The other two approaches ("die" and "error and usage") can be argued
for, but this one ("give usage") just seems wrong to me. I haven't
looked for such a place in the code, and maybe they're "obvious", but it
seems odd to just give the usage without any sort of hint about what was
wrong.

>  check-attr has a private error_with_usage() helper to implement
> the error() followed by usage_with_options() idiom.  Most callers just
> use die(), like
>
> die(_("'%s' cannot be used with %s"), "--merge", "--patch");
>
> Documentation/technical/api-error-handling.txt says
>
>  - `usage` is for errors in command line usage.  After printing its
>message, it exits with status 129.  (See also `usage_with_options`
>in the link:api-parse-options.html[parse-options API].)
>
> which is not prescriptive enough to help.

I think it would be a larger project to make these consistent. The one
I'm adding here is at least consistent with the other one in this file.

> Separate from that, I wonder if the error message can be made a little
> shorter and clearer.  E.g.
>
> fatal: shortlog  can only be used inside a git repository

Some grepping suggests we do not usually name the command ("shortlog
..."), perhaps to play well with aliasing, nor do we use "such "
very often, but it does happen. Quoting and allowing for options might
make this more correct, but perhaps less readable: "'shortlog [...]
' can only ...". Slightly better than what I had, "revisions can
only be given inside a git repository" would avoid some negating.

> Thanks and hope that helps,

It does indeed. I'll give this another 24h and see what I come up with.
I believe it will end up in a change to "<= 1", an improved error
message and a clearer last few words in the commit message.

Martin

[PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

[Ævar Arnfjörð Bjarmason]

Add a INSTALL_SYMLINKS option which if enabled, changes the default
hardlink installation method to one where the relevant binaries in
libexec/git-core are symlinked back to ../../bin, instead of being
hardlinked.

This new option also overrides the behavior of the existing
NO_*_HARDLINKS variables which in some cases would produce symlinks
within to libexec/, e.g. "git-add" symlinked to "git" which would be
copy of the "git" found in bin/, now "git-add" in libexec/ is always
going to be symlinked to the "git" found in the bin/ directory.

This option is being added because:

 1) I think it makes what we're doing a lot more obvious. E.g. I'd
never noticed that the libexec binaries were really just hardlinks
since e.g. ls(1) won't show that in any obvious way. You need to
start stat(1)-ing things and look at the inodes to see what's
going on.

 2) Some tools have very crappy support for hardlinks, e.g. the Git
shipped with GitLab is much bigger than it should be because
they're using a chef module that doesn't know about hardlinks, see
https://github.com/chef/omnibus/issues/827

I've also ran into other related issues that I think are explained
by this, e.g. compiling git with debugging and rpm refusing to
install a ~200MB git package with 2GB left on the FS, I think that
was because it doesn't consider hardlinks, just the sum of the
byte size of everything in the package.

As for the implementation, the "../../bin" noted above will vary given
some given some values of "../.." and "bin" depending on the depth of
the gitexecdir relative to the destdir, and the "bindir" target,
e.g. setting "bindir=/tmp/git/binaries gitexecdir=foo/bar/baz" will do
the right thing and produce this result:

$ file /tmp/git/foo/bar/baz/git-add
/tmp/git/foo/bar/baz/git-add: symbolic link to ../../../binaries/git

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 46 +++---
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index ee0b6c8940..ac7616422d 100644
--- a/Makefile
+++ b/Makefile
@@ -329,6 +329,13 @@ all::
 # when hardlinking a file to another name and unlinking the original file right
 # away (some NTFS drivers seem to zero the contents in that scenario).
 #
+# Define INSTALL_SYMLINKS if you prefer to have everything that can be
+# symlinked between bin/ and libexec/ to use relative symlinks between
+# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and
+# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
+# within the same directory in some cases, INSTALL_SYMLINKS will
+# always symlink to the final target directly.
+#
 # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
 # programs as a tar, where bin/ and libexec/ might be on different file 
systems.
 #
@@ -2594,35 +2601,44 @@ endif
 
bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
+   destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 
's|[^/][^/]*|..|g') && \
{ test "$$bindir/" = "$$execdir/" || \
  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); 
do \
$(RM) "$$execdir/$$p" && \
-   test -z 
"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
-   ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
-   cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
+   test -n "$(INSTALL_SYMLINKS)" && \
+   ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" 
"$$execdir/$$p" || \
+   { test -z 
"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
+ ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
+ cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \
  done; \
} && \
for p in $(filter $(install_bindir_programs),$(BUILT_INS)); do \
$(RM) "$$bindir/$$p" && \
-   test -z "$(NO_INSTALL_HARDLINKS)" && \
-   ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
-   ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
-   cp "$$bindir/git$X" "$$bindir/$$p" || exit; \
+   test -n "$(INSTALL_SYMLINKS)" && \
+   ln -s "git$X" "$$bindir/$$p" || \
+   { test -z "$(NO_INSTALL_HARDLINKS)" && \
+ ln "$$bindir/git$X" "$$bindir/$$p" 2>/dev/null || \
+ ln -s "git$X" "$$bindir/$$p" 2>/dev/null || \
+ cp "$$bindir/git$X" "$$bindir/$$p" || exit; } \
done && \
for p in $(BUILT_INS); do \
$(RM) "$$execdir/$$p" && \
-   test -z "$(NO_INSTALL_HARDLINKS)" && \
-   ln "$$execdir/git$X" "$$execdir/$$p" 2>/dev/null || \
-   ln -s "git$X" "$$execdir/$$p" 2>/dev/null || \
-   cp 

[PATCH 2/3] Makefile: add a gitexecdir_relative variable

[Ævar Arnfjörð Bjarmason]

This variable will be e.g. "libexec/git-core" if
gitexecdir=/tmp/git/libexec/git-core is given. It'll be used by a
subsequent change.

This is stolen from the yet-to-be integrated (needs resubmission)
"Makefile: add Perl runtime prefix support" patch on the mailing
list. See
<20180108030239.92036-3-...@google.com> 
(https://public-inbox.org/git/20180108030239.92036-3-...@google.com/).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Makefile b/Makefile
index b2f8f2b171..ee0b6c8940 100644
--- a/Makefile
+++ b/Makefile
@@ -488,6 +488,7 @@ pathsep = :
 bindir_relative = $(patsubst $(prefix)/%,%,$(bindir))
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
+gitexecdir_relative = $(patsubst $(prefix)/%,%,$(gitexecdir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
 
 export prefix bindir sharedir sysconfdir gitwebdir perllibdir localedir
@@ -1735,6 +1736,7 @@ infodir_relative_SQ = $(subst ','\'',$(infodir_relative))
 perllibdir_SQ = $(subst ','\'',$(perllibdir))
 localedir_SQ = $(subst ','\'',$(localedir))
 gitexecdir_SQ = $(subst ','\'',$(gitexecdir))
+gitexecdir_relative_SQ = $(subst ','\'',$(gitexecdir_relative))
 template_dir_SQ = $(subst ','\'',$(template_dir))
 htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative))
 prefix_SQ = $(subst ','\'',$(prefix))
-- 
2.15.1.424.g9478a66081

[PATCH 1/3] Makefile: fix broken bindir_relative variable

[Ævar Arnfjörð Bjarmason]

Change the bindir_relative variable to work like the other *_relative
variables, which are computed as a function of the absolute
path. Before this change, supplying e.g. bindir=/tmp/git/binaries to
the Makefile would yield a bindir_relative of just "bin", as opposed
to "binaries".

This logic was originally added back in 026fa0d5ad ("Move computation
of absolute paths from Makefile to runtime (in preparation for
RUNTIME_PREFIX)", 2009-01-18), then later in 971f85388f ("Makefile:
make mandir, htmldir and infodir absolute", 2013-02-24) when
more *_relative variables were added those new variables didn't have
this bug, but bindir_relative was never fixed.

There is a small change in behavior here, which is that setting
bindir_relative as an argument to the Makefile won't work anymore, I
think that's fine, since this was always intended as an internal
variable (e.g. INSTALL documents bindir=*, not bindir_relative=*).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index de4b8f0c02..b2f8f2b171 100644
--- a/Makefile
+++ b/Makefile
@@ -468,8 +468,7 @@ ARFLAGS = rcs
 # This can help installing the suite in a relocatable way.
 
 prefix = $(HOME)
-bindir_relative = bin
-bindir = $(prefix)/$(bindir_relative)
+bindir = $(prefix)/bin
 mandir = $(prefix)/share/man
 infodir = $(prefix)/share/info
 gitexecdir = libexec/git-core
@@ -486,6 +485,7 @@ lib = lib
 # DESTDIR =
 pathsep = :
 
+bindir_relative = $(patsubst $(prefix)/%,%,$(bindir))
 mandir_relative = $(patsubst $(prefix)/%,%,$(mandir))
 infodir_relative = $(patsubst $(prefix)/%,%,$(infodir))
 htmldir_relative = $(patsubst $(prefix)/%,%,$(htmldir))
-- 
2.15.1.424.g9478a66081

[PATCH 0/3] Makefile: add a INSTALL_SYMLINKS option

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Junio C. Hamano jotted:

> Ævar Arnfjörð Bjarmason  writes:
>
>> Related to this, I came across this bug report
>> https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
>> wondering why we're installing N copies of the git binary, presumably
>> they're building with NO_INSTALL_HARDLINKS.
>> ...
>> But is there any reason anyone can think of for why we shouldn't be
>> figuring out the relative path and symlinking the two?
>
>
> There is no fundamental reason not to offer such an "install" method
> as an option; unless you count a more philosophical aversion to use
> symlinks due to (perceived) additional fragility, that is.
>
> The resulting code may become messier than without, but as long as
> it is without the reasonable range for usual price we would pay for
> a new "feature", that would be tolerable, I guess.

Cool. I think it makes sense for us to have this. Here's an
implementation of it. The 3/3 patch looks a bit scary, but "git show"
with --word-diff will show that the change is minimal.

This steals a small piece from Daniel's relocatable series, and
doesn't in any way conflict with it. None of this will need to be
fixed up to make git relocatable since all the symlinks are relative
already.

Ævar Arnfjörð Bjarmason (3):
  Makefile: fix broken bindir_relative variable
  Makefile: add a gitexecdir_relative variable
  Makefile: optionally symlink libexec/git-core binaries to bin/git

 Makefile | 52 +++-
 1 file changed, 35 insertions(+), 17 deletions(-)

-- 
2.15.1.424.g9478a66081

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Stan Hu jotted:

> To be clear, this is how I think we got into this state:
>
> We have worktrees that are created to squash and rebase a branch. They were 
> left around inadvertently for one reason or another.
>
> Since we were using git v2.14.3, a git gc would prune dangling objects 
> because it never saw that a worktree still had a reference to it. Now, in git 
> v2.15+, worktrees are examined, and a git gc won't garbage collect any 
> objects referenced by a worktree.
>
> Another words, the failure of git v2.14 to preserve these objects caused 
> problems with these stale worktrees with the upgrade to v2.15+.

Thanks, I hadn't dug into the root cause. Good to know it's not some
failure mode we're still producing, just the result of past bugs.

[GSoC] [PATCH] test: avoid pipes in git related commands for test suite

[Pratik Karki]

This patch removes the necessity of pipes in git related commands for test 
suite.

Exit code of the upstream in a pipe is ignored so, it's use should be avoided. 
The fix for this is to write the output of the git command to a file and test 
the exit codes of both the commands being linked by pipe.

Signed-off-by: Pratik Karki 
---
 t/t7001-mv.sh| 24 
 t/t9104-git-svn-follow-parent.sh |  4 ++--
 t/t9110-git-svn-use-svm-props.sh | 36 ++--
 t/t9111-git-svn-use-svnsync-props.sh | 36 ++--
 t/t9114-git-svn-dcommit-merge.sh |  8 
 t/t9130-git-svn-authors-file.sh  | 16 
 t/t9138-git-svn-authors-prog.sh  | 28 ++--
 t/t9153-git-svn-rewrite-uuid.sh  |  8 
 8 files changed, 80 insertions(+), 80 deletions(-)

diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh
index 6e5031f56..0dcf1fa3e 100755
--- a/t/t7001-mv.sh
+++ b/t/t7001-mv.sh
@@ -21,8 +21,8 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-grep "^R100..*path0/COPYING..*path1/COPYING"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+grep "^R100..*path0/COPYING..*path1/COPYING" actual'
 
 test_expect_success \
 'moving the file back into subdirectory' \
@@ -35,8 +35,8 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
-grep "^R100..*path1/COPYING..*path0/COPYING"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+grep "^R100..*path1/COPYING..*path0/COPYING" actual'
 
 test_expect_success \
 'checking -k on non-existing file' \
@@ -116,10 +116,10 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path0/COPYING..*path2/COPYING" &&
- git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path0/README..*path2/README"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path0/COPYING..*path2/COPYING" actual &&
+ git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path0/README..*path2/README" actual'
 
 test_expect_success \
 'succeed when source is a prefix of destination' \
@@ -135,10 +135,10 @@ test_expect_success \
 
 test_expect_success \
 'checking the commit' \
-'git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path2/COPYING..*path1/path2/COPYING" &&
- git diff-tree -r -M --name-status  HEAD^ HEAD | \
- grep "^R100..*path2/README..*path1/path2/README"'
+'git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path2/COPYING..*path1/path2/COPYING" actual &&
+ git diff-tree -r -M --name-status  HEAD^ HEAD >actual &&
+ grep "^R100..*path2/README..*path1/path2/README" actual'
 
 test_expect_success \
 'do not move directory over existing directory' \
diff --git a/t/t9104-git-svn-follow-parent.sh b/t/t9104-git-svn-follow-parent.sh
index cd480edf1..284d1224e 100755
--- a/t/t9104-git-svn-follow-parent.sh
+++ b/t/t9104-git-svn-follow-parent.sh
@@ -204,8 +204,8 @@ test_expect_success "follow-parent is atomic" '
 test_expect_success "track multi-parent paths" '
svn_cmd cp -m "resurrect /glob" "$svnrepo"/r9270 "$svnrepo"/glob &&
git svn multi-fetch &&
-   test $(git cat-file commit refs/remotes/glob | \
-  grep "^parent " | wc -l) -eq 2
+   test $(git cat-file commit refs/remotes/glob >actual &&
+  grep "^parent " actual | wc -l) -eq 2
'
 
 test_expect_success "multi-fetch continues to work" "
diff --git a/t/t9110-git-svn-use-svm-props.sh b/t/t9110-git-svn-use-svm-props.sh
index dde0a3c22..a1a00c298 100755
--- a/t/t9110-git-svn-use-svm-props.sh
+++ b/t/t9110-git-svn-use-svm-props.sh
@@ -21,32 +21,32 @@ uuid=161ce429-a9dd-4828-af4a-52023f968c89
 
 bar_url=http://mayonaise/svnrepo/bar
 test_expect_success 'verify metadata for /bar' "
-   git cat-file commit refs/remotes/bar | \
-  grep '^git-svn-id: $bar_url@12 $uuid$' &&
-   git cat-file commit refs/remotes/bar~1 | \
-  grep '^git-svn-id: $bar_url@11 $uuid$' &&
-   git cat-file commit refs/remotes/bar~2 | \
-  grep '^git-svn-id: $bar_url@10 $uuid$' &&
-   git cat-file commit refs/remotes/bar~3 | \
-  grep '^git-svn-id: $bar_url@9 $uuid$' &&
-   git cat-file commit refs/remotes/bar~4 | \
-  grep '^git-svn-id: $bar_url@6 $uuid$' &&
-   git cat-file commit refs/remotes/bar~5 | \
-  grep '^git-svn-id: $bar_url@1 $uuid$'
+   git cat-file commit refs/remotes/bar >actual &&
+  grep '^git-svn-id: $bar_url@12 $uuid$' actual &&
+   git cat-file commit refs/remotes/bar~1 >actual &&
+  grep '^git-svn-id: 

[PATCH] Documentation/githooks: Clarify the behavior of post-checkout hook

[Magne Land]

From: Magne Land 

This can happen when using 'git rebase -i’:
could not detach HEAD

Based on discovering this Stack Overflow discussion:
https://stackoverflow.com/questions/25561485/git-rebase-i-with-squash-cannot-detach-head
---
 Documentation/githooks.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index f877f7b7cd19c..3a4e027d0d175 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt
@@ -166,7 +166,9 @@ worktree.  The hook is given three parameters: the ref of 
the previous HEAD,
 the ref of the new HEAD (which may or may not have changed), and a flag
 indicating whether the checkout was a branch checkout (changing branches,
 flag=1) or a file checkout (retrieving a file from the index, flag=0).
-This hook cannot affect the outcome of 'git checkout'.
+
+If this hook exits with a non-zero status, 'git checkout' will exit with the
+same status.
 
 It is also run after 'git clone', unless the --no-checkout (-n) option is
 used. The first parameter given to the hook is the null-ref, the second the

--
https://github.com/git/git/pull/470

Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Jonathan Nieder]

Hi,

Martin Ågren wrote:

> If we are outside a repo and have any arguments left after
> option-parsing, `setup_revisions()` will try to do its job and
> something like this will happen:
>
>  $ git shortlog v2.16.0..
>  BUG: environment.c:183: git environment hasn't been setup
>  Aborted (core dumped)

Yikes.  Thanks for fixing it.

[...]
>   (So yes, after
> this patch, we will still silently ignore stdin for confused usage such
> as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does
> not crash.)

I don't follow here.  Are you saying this command should notice that
there is input in stdin?  How would it notice?

[...]
> --- a/t/t4201-shortlog.sh
> +++ b/t/t4201-shortlog.sh
> @@ -127,6 +127,11 @@ test_expect_success !MINGW 'shortlog can read 
> --format=raw output' '
>   test_cmp expect out
>  '
>  
> +test_expect_success 'shortlog from non-git directory refuses revisions' '
> + test_must_fail env GIT_DIR=non-existing git shortlog HEAD 2>out &&
> + test_i18ngrep "no revisions can be given" out
> +'

\o/

[...]
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -293,6 +293,12 @@ int cmd_shortlog(int argc, const char **argv, const char 
> *prefix)
>  parse_done:
>   argc = parse_options_end();
>  
> + if (nongit && argc != 1) {

Just curious: would argc ever be 0 here?  'argc <= 1' might be clearer.

> + error(_("no revisions can be given when running "
> + "from outside a repository"));
> + usage_with_options(shortlog_usage, options);
> + }
> +

The error message is

error: no revisions can be given when running from outside a repository
usage: ...

Do we need to dump usage here?  I wonder if a simple die() call would
be easier for the user to act on.

Not about this patch: I was a little surprised to see 'error:' instead
of 'usage:' or 'fatal:'.  It turns out git is pretty inconsistent
about that: e.g. there is

error(_("no remote specified"));
usage_with_options(builtin_remote_setbranches_usage, options);

Some other callers just use usage_with_options without describing the
error.  check-attr has a private error_with_usage() helper to implement
the error() followed by usage_with_options() idiom.  Most callers just
use die(), like

die(_("'%s' cannot be used with %s"), "--merge", "--patch");

Documentation/technical/api-error-handling.txt says

 - `usage` is for errors in command line usage.  After printing its
   message, it exits with status 129.  (See also `usage_with_options`
   in the link:api-parse-options.html[parse-options API].)

which is not prescriptive enough to help.

Separate from that, I wonder if the error message can be made a little
shorter and clearer.  E.g.

fatal: shortlog  can only be used inside a git repository

Thanks and hope that helps,
Jonathan

Re: [PATCH v4 01/35] pkt-line: introduce packet_read_with_status

[Brandon Williams]

On 03/13, Jonathan Tan wrote:
> On Wed, 28 Feb 2018 15:22:18 -0800
> Brandon Williams  wrote:
> 
> > +   if (len < 0) {
> > die("protocol error: bad line length character: %.4s", linelen);
> > -   if (!len) {
> > +   } else if (!len) {
> > packet_trace("", 4, 0);
> > -   return 0;
> > +   return PACKET_READ_FLUSH;
> > +   } else if (len < 4) {
> > +   die("protocol error: bad line length %d", len);
> > }
> > +
> > len -= 4;
> > -   if (len >= size)
> > +   if ((unsigned)len >= size)
> > die("protocol error: bad line length %d", len);
> 
> The cast to unsigned is safe, since len was at least 4 before "len -=
> 4". I can't think of a better way to write this to make that more
> obvious, though.
> 
> > +/*
> > + * Read a packetized line into a buffer like the 'packet_read()' function 
> > but
> > + * returns an 'enum packet_read_status' which indicates the status of the 
> > read.
> > + * The number of bytes read will be assigined to *pktlen if the status of 
> > the
> > + * read was 'PACKET_READ_NORMAL'.
> > + */
> > +enum packet_read_status {
> > +   PACKET_READ_EOF = -1,
> > +   PACKET_READ_NORMAL,
> > +   PACKET_READ_FLUSH,
> > +};
> > +enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> > +   size_t *src_len, char *buffer,
> > +   unsigned size, int *pktlen,
> > +   int options);
> 
> jrnieder said in [1], referring to the definition of enum
> packet_read_status:
> 
> > nit: do any callers treat the return value as a number?  It would be
> > less magical if the numbering were left to the compiler (0, 1, 2).

yeah i'll do this.

> 
> I checked the result of the entire patch set and the only callers seem
> to be packet_read() (modified in this patch) and the
> soon-to-be-introduced packet_reader_read(). So not only can the
> numbering be left to the compiler, this function can (and should) be
> marked static as well (and the enum definition moved to .c), since I
> think that future development should be encouraged to use packet_reader.

The enum definition can't be moved as its needed outside this file.

> 
> The commit message would also thus need to be rewritten, since this
> becomes more of a refactoring into a function with a more precisely
> specified return type, to be used both by the existing packet_read() and
> a soon-to-be-introduced packet_reader_read().
> 
> [1] 
> https://public-inbox.org/git/20180213002554.ga42...@aiede.svl.corp.google.com/

-- 
Brandon Williams

Re: [PATCH v4 04/35] upload-pack: convert to a builtin

[Brandon Williams]

On 03/13, Jonathan Tan wrote:
> On Wed, 28 Feb 2018 15:22:21 -0800
> Brandon Williams  wrote:
> 
> > In order to allow for code sharing with the server-side of fetch in
> > protocol-v2 convert upload-pack to be a builtin.
> > 
> > Signed-off-by: Brandon Williams 
> 
> I suggested updating the commit message in my previous review [1], but I
> understand that my comment might have been lost in the ensuing long
> discussion.
> 
> [1] 
> https://public-inbox.org/git/20180221134422.2386e1aca39fe67323559...@google.com/

Your suggested change to the commit msg isn't quite accurate as you can
already run "git-upload-pack --help" today.

-- 
Brandon Williams

Re: [PATCH v4 01/35] pkt-line: introduce packet_read_with_status

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:18 -0800
Brandon Williams  wrote:

> + if (len < 0) {
>   die("protocol error: bad line length character: %.4s", linelen);
> - if (!len) {
> + } else if (!len) {
>   packet_trace("", 4, 0);
> - return 0;
> + return PACKET_READ_FLUSH;
> + } else if (len < 4) {
> + die("protocol error: bad line length %d", len);
>   }
> +
>   len -= 4;
> - if (len >= size)
> + if ((unsigned)len >= size)
>   die("protocol error: bad line length %d", len);

The cast to unsigned is safe, since len was at least 4 before "len -=
4". I can't think of a better way to write this to make that more
obvious, though.

> +/*
> + * Read a packetized line into a buffer like the 'packet_read()' function but
> + * returns an 'enum packet_read_status' which indicates the status of the 
> read.
> + * The number of bytes read will be assigined to *pktlen if the status of the
> + * read was 'PACKET_READ_NORMAL'.
> + */
> +enum packet_read_status {
> + PACKET_READ_EOF = -1,
> + PACKET_READ_NORMAL,
> + PACKET_READ_FLUSH,
> +};
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> + size_t *src_len, char *buffer,
> + unsigned size, int *pktlen,
> + int options);

jrnieder said in [1], referring to the definition of enum
packet_read_status:

> nit: do any callers treat the return value as a number?  It would be
> less magical if the numbering were left to the compiler (0, 1, 2).

I checked the result of the entire patch set and the only callers seem
to be packet_read() (modified in this patch) and the
soon-to-be-introduced packet_reader_read(). So not only can the
numbering be left to the compiler, this function can (and should) be
marked static as well (and the enum definition moved to .c), since I
think that future development should be encouraged to use packet_reader.

The commit message would also thus need to be rewritten, since this
becomes more of a refactoring into a function with a more precisely
specified return type, to be used both by the existing packet_read() and
a soon-to-be-introduced packet_reader_read().

[1] 
https://public-inbox.org/git/20180213002554.ga42...@aiede.svl.corp.google.com/

RE: Why don't we symlink libexec/git-core/* to bin/git?

[Randall S. Becker]

On March 13, 2018 2:37 PM, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  writes:
> 
> > Related to this, I came across this bug report
> > https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
> > wondering why we're installing N copies of the git binary, presumably
> > they're building with NO_INSTALL_HARDLINKS.
> > ...
> > But is there any reason anyone can think of for why we shouldn't be
> > figuring out the relative path and symlinking the two?
> 
> 
> There is no fundamental reason not to offer such an "install" method as an
> option; unless you count a more philosophical aversion to use symlinks due
> to (perceived) additional fragility, that is.
> 
> The resulting code may become messier than without, but as long as it is
> without the reasonable range for usual price we would pay for a new
> "feature", that would be tolerable, I guess.

A possible (remote) reason for not doing this is in environments using ACLs 
that somehow want different access permissions on some functions vs. others AND 
where the platform does not have the ability to separately secure links vs. 
objects. I don't know of such an environment, but you never know. I know it's a 
stretch, but I can see security-types being worried about this. I do know of 
environments where /usr/local/lib is secured differently from /usr/local/bin to 
prevent inappropriate .so loads on a selective basis, so there's that. Again, 
this is a stretch. As long as we continue to have a method of forcing the 
expensive way for the paranoidly inclined ;)-- not meaning offence to 
those, of course.

Cheers,
Randall

-- Brief whoami:
 NonStop developer since approximately 2112884442
 UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [RFC 0/4] ref-filter: remove printing from formatting logic

[Martin Ågren]

Hi Olga

On 13 March 2018 at 11:25, Оля Тележная  wrote:
> The main idea of the patch is, if you want to format the output by
> ref-filter, you should have an ability to work with errors by yourself
> if you want to.
> So I decided not to touch signature of show_ref_array_item(), but to
> move all printing (I mean errors) to it. So that we could invoke
> format_ref_array_item() and be sure that we cold handle errors by
> ourselves.
>
> The patch is not finished, but I decided to show it to you. There are
> still many places where we could die in the middle of formatting
> process. But, if you like the general idea, I will finish and re-send
> it.
>
> Another question is about verify_ref_format(). Do we need to allow its
> users also to manage errors by themselves? I left the old scenario,
> printing everything in verify_ref_format() and die. If you have better
> ideas, please share them.

I think it is a good idea to stop die-ing in "libgit". This seems like a
good way of achieving that, or isolating the issue. Do you have any
particular use-case for this, i.e., are you setting up the stage for a
patch "5" where you add a new user of one of these?

I do wonder whether a helper function to call strbuf_addstr() and return
-1 would be a good idea though. I mentioned it in patch 2, then with
patches 3 and 4, it started to seem like a reasonably good idea. It
would be a shame if this sort of "boilerplate" for handling errors could
have an impact on code clarity / obviousness.

Another issue is whether passing NULL for an error-strbuf should be a
way of saying "I don't care; die() so I do not have to". Well, right now
I guess passing NULL would indeed terminate the program. ;-) Such a
construct might be another reason for providing error_strbuf_addstr()...
Of course, it also means we keep die-ing in libgit..

I feel I'm just talking out loud. Maybe you find my input useful.

Martin

Re: [PATCH 1/3] git-shortlog.txt: reorder usages

[Junio C Hamano]

Martin Ågren  writes:

> The first usage we give is the original one where, e.g., `git log` is
> piped through `git shortlog`. The description that follows reads the
> other way round, by first focusing on the general behavior, then ending
> with the behavior when reading from stdin.
> ...

The result looks a lot more useful than without the patch.  I think
the current ordering is merely a historical accident made in 2006.

Will queue; thanks.

> diff --git a/Documentation/git-shortlog.txt b/Documentation/git-shortlog.txt
> index ee6c5476c..5e35ea18a 100644
> --- a/Documentation/git-shortlog.txt
> +++ b/Documentation/git-shortlog.txt
> @@ -8,8 +8,8 @@ git-shortlog - Summarize 'git log' output
>  SYNOPSIS
>  
>  [verse]
> -git log --pretty=short | 'git shortlog' []
>  'git shortlog' [] [] [[\--] ...]
> +git log --pretty=short | 'git shortlog' []
>  
>  DESCRIPTION
>  ---

Re: [RFC 3/4] ref-filter: change parsing function error handling

[Martin Ågren]

On 13 March 2018 at 11:16, Olga Telezhnaya  wrote:
> Continue removing any printing from ref-filter formatting logic,
> so that it could be more general.
>
> Change the signature of parse_ref_filter_atom() by changing return value,
> adding previous return value to function parameter and also adding
> strbuf parameter for error message.

I think the current return value is always non-negative. Maybe it would
be easier to leave the return value as-is, except return negative on
error? Unless I am missing something?

>
> Signed-off-by: Olga Telezhnaia 
> ---
>  ref-filter.c | 45 -
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 07bedc636398c..e146215bf1e64 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -397,7 +397,8 @@ struct atom_value {
>   * Used to parse format string and sort specifiers
>   */
>  static int parse_ref_filter_atom(const struct ref_format *format,
> -const char *atom, const char *ep)
> +const char *atom, const char *ep, int *res,
> +struct strbuf *err)
>  {
> const char *sp;
> const char *arg;
> @@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct 
> ref_format *format,
> sp = atom;
> if (*sp == '*' && sp < ep)
> sp++; /* deref */
> -   if (ep <= sp)
> -   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
> +   if (ep <= sp) {
> +   strbuf_addf(err, _("malformed field name: %.*s"), 
> (int)(ep-atom), atom);
> +   return -1;
> +   }
>
> /* Do we have the atom already used elsewhere? */
> for (i = 0; i < used_atom_cnt; i++) {
> int len = strlen(used_atom[i].name);
> -   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
> -   return i;
> +   if (len == ep - atom && !memcmp(used_atom[i].name, atom, 
> len)) {
> +   *res = i;
> +   return 0;
> +   }
> }

If you did so, this hunk above would not need to be changed ...

> @@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format 
> *format,
> need_tagged = 1;
> if (!strcmp(valid_atom[i].name, "symref"))
> need_symref = 1;
> -   return at;
> +   *res = at;
> +   return 0;
>  }

... nor this one above ...

> if (!ep)
> return error(_("malformed format string %s"), sp);
> /* sp points at "%(" and ep points at the closing ")" */
> -   at = parse_ref_filter_atom(format, sp + 2, ep);
> +   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
> +   die("%s", err.buf);

And this would be more like "if (at < 0) die(...)".

> for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
> struct atom_value *atomv;
> +   struct strbuf err = STRBUF_INIT;
> +   int pos;
>
> ep = strchr(sp, ')');
> if (cp < sp)
> append_literal(cp, sp, );
> -   get_ref_atom_value(info,
> -  parse_ref_filter_atom(format, sp + 2, ep),
> -  );
> +   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
> +   return -1;
> +   get_ref_atom_value(info, pos, );
> if (atomv->handler(atomv, , error_buf))
> return -1;
> +   strbuf_release();

This looks leaky: if we get an error, we've got something in the buffer
but we do not release it because we return early. Stepping back a bit, I
wonder why we do not do anything at all with "err". Stepping back a bit
more :-) I wonder if you could get rid of "err" and pass "error_buf" to
parse_ref_filter_atom() instead. Our caller would like to have access to
the error string?

This ties back to my comment on the first patch -- "return negative if
and only if you add some error string to the buffer" might be a useful
rule?

Martin

Re: [RFC 2/4] ref-filter: add return value && strbuf to handlers

[Martin Ågren]

On 13 March 2018 at 11:16, Olga Telezhnaya  wrote:
> -static void append_atom(struct atom_value *v, struct ref_formatting_state 
> *state)
> +static int append_atom(struct atom_value *v, struct ref_formatting_state 
> *state,
> +  struct strbuf *err)
>  {
> /*
>  * Quote formatting is only done when the stack has a single
> @@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct 
> ref_formatting_state *state
> quote_formatting(>stack->output, v->s, 
> state->quote_style);
> else
> strbuf_addstr(>stack->output, v->s);
> +   return 0;
>  }

Maybe "unused_err" instead of "err", to document that we are aware that
"err" is not being used and that it is ok. Something similar is done in
builtin/difftool.c.

> -static void then_atom_handler(struct atom_value *atomv, struct 
> ref_formatting_state *state)
> +static int then_atom_handler(struct atom_value *atomv, struct 
> ref_formatting_state *state,
> +struct strbuf *err)
>  {
> struct ref_formatting_stack *cur = state->stack;
> struct if_then_else *if_then_else = NULL;
>
> if (cur->at_end == if_then_else_handler)
> if_then_else = (struct if_then_else *)cur->at_end_data;
> -   if (!if_then_else)
> -   die(_("format: %%(then) atom used without an %%(if) atom"));
> -   if (if_then_else->then_atom_seen)
> -   die(_("format: %%(then) atom used more than once"));
> -   if (if_then_else->else_atom_seen)
> -   die(_("format: %%(then) atom used after %%(else)"));
> +   if (!if_then_else) {
> +   strbuf_addstr(err, _("format: %(then) atom used without an 
> %(if) atom"));
> +   return -1;
> +   } else if (if_then_else->then_atom_seen) {
> +   strbuf_addstr(err, _("format: %(then) atom used more than 
> once"));
> +   return -1;
> +   } else if (if_then_else->else_atom_seen) {
> +   strbuf_addstr(err, _("format: %(then) atom used after 
> %(else)"));
> +   return -1;
> +   }

I slowly start to wonder if we want a function (or macro)
error_strbuf_addstr(), which returns -1. That could probably wait,
though.

Martin

Re: [RFC 1/4] ref-filter: start adding strbufs with errors

[Martin Ågren]

On 13 March 2018 at 11:16, Olga Telezhnaya  wrote:
> This is a first step in removing any printing from
> ref-filter formatting logic, so that it could be more general.
> Everything would be the same for show_ref_array_item() users.
> But, if you want to deal with errors by your own, you could invoke
> format_ref_array_item(). It means that you need to print everything
> (the result and errors) on your side.
>
> This commit changes signature of format_ref_array_item() by adding
> return value and strbuf parameter for errors, and fixes
> its callers.

Minor nit: Maybe s/fixes its callers/adjusts its callers/. They are not
broken or need to be fixed. They were simply playing the game according
to the old rules, and now they need to learn the new ways. :-)

> Signed-off-by: Olga Telezhnaia 
> ---
>  builtin/branch.c |  7 +--
>  ref-filter.c | 17 -
>  ref-filter.h |  7 ---
>  3 files changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 8dcc2ed058be6..f86709ca42d5e 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, 
> struct ref_sorting *sortin
> struct ref_array array;
> int maxwidth = 0;
> const char *remote_prefix = "";
> -   struct strbuf out = STRBUF_INIT;

You move this variable into the loop to reduce its scope. At first I
suspected that this might mean we now start allocating+releasing in each
run of the loop, which might be a performance-regression. But it turns
out, we already did that, so this tightening of the scope has no such
downsides. :-) From the commit message, I wasn't expecting this move,
though. Maybe "While at it, reduce the scope of the out-variable."

> char *to_free = NULL;
>
> /*
> @@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, 
> struct ref_sorting *sortin
> ref_array_sort(sorting, );
>
> for (i = 0; i < array.nr; i++) {
> -   format_ref_array_item(array.items[i], format, );
> +   struct strbuf out = STRBUF_INIT;
> +   struct strbuf err = STRBUF_INIT;
> +   if (format_ref_array_item(array.items[i], format, , ))
> +   die("%s", err.buf);

Using "%s", good.

> if (column_active(colopts)) {
> assert(!filter->verbose && "--column and --verbose 
> are incompatible");
>  /* format to a string_list to let print_columns() do 
> its job */
> @@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, 
> struct ref_sorting *sortin
> fwrite(out.buf, 1, out.len, stdout);
> putchar('\n');
> }
> +   strbuf_release();
> strbuf_release();
> }
>
> diff --git a/ref-filter.c b/ref-filter.c
> index 45fc56216aaa8..54fae00bdd410 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char 
> *ep, struct ref_formatting
> }
>  }
>
> -void format_ref_array_item(struct ref_array_item *info,
> +int format_ref_array_item(struct ref_array_item *info,
>const struct ref_format *format,
> -  struct strbuf *final_buf)
> +  struct strbuf *final_buf,
> +  struct strbuf *error_buf)
>  {
> const char *cp, *sp, *ep;
> struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
> @@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item 
> *info,
> resetv.s = GIT_COLOR_RESET;
> append_atom(, );
> }
> -   if (state.stack->prev)
> -   die(_("format: %%(end) atom missing"));
> +   if (state.stack->prev) {
> +   strbuf_addstr(error_buf, _("format: %(end) atom missing"));
> +   return -1;
> +   }
> strbuf_addbuf(final_buf, >output);
> pop_stack_element();
> +   return 0;
>  }
>
>  void show_ref_array_item(struct ref_array_item *info,
>  const struct ref_format *format)
>  {
> struct strbuf final_buf = STRBUF_INIT;
> +   struct strbuf error_buf = STRBUF_INIT;
>
> -   format_ref_array_item(info, format, _buf);
> +   if (format_ref_array_item(info, format, _buf, _buf))
> +   die("%s", error_buf.buf);
> fwrite(final_buf.buf, 1, final_buf.len, stdout);
> +   strbuf_release(_buf);

I think this `strbuf_release()` will never actually do anything. If we
get here, we had no error. But it makes sense (to me) to always be clear
about releasing this. In this case it is easy enough.

Possible counterargument: We might want this sort of "error-handling by
strbufs" to follow this simple rule: return an error if and only if 

Re: [PATCH v5] fetch-pack.c: use oidset to check existence of loose object

[Junio C Hamano]

Takuto Ikuta  writes:

> +struct loose_object_iter {
> + struct oidset *loose_object_set;
> + struct ref *refs;
> +};
> +
> +/*
> + *  If the number of refs is not larger than the number of loose objects,
> + *  this function stops inserting and returns false.
> + */
> +static int add_loose_objects_to_set(const struct object_id *oid,
> + const char *path,
> + void *data)
> +{
> + struct loose_object_iter *iter = data;
> + oidset_insert(iter->loose_object_set, oid);
> + if (iter->refs == NULL)
> + return 1;
> +
> + iter->refs = iter->refs->next;
> + return 0;
> +}

That's clever.  By traversing the refs list as you iterate over the
loose objects, you do not do any unnecessary work ;-)

Thanks for working on this.

> @@ -719,16 +741,31 @@ static int everything_local(struct fetch_pack_args 
> *args,
>   int retval;
>   int old_save_commit_buffer = save_commit_buffer;
>   timestamp_t cutoff = 0;
> + struct oidset loose_oid_set = OIDSET_INIT;
> + int use_oidset = 0;
> + struct loose_object_iter iter = {_oid_set, *refs};
> +
> + /* Enumerate all loose objects or know refs are not so many. */
> + use_oidset = !for_each_loose_object(add_loose_objects_to_set,
> + , 0);
>  
>   save_commit_buffer = 0;
>  
>   for (ref = *refs; ref; ref = ref->next) {
>   struct object *o;
> + unsigned int flags = OBJECT_INFO_QUICK;
>  
> - if (!has_object_file_with_flags(>old_oid,
> - OBJECT_INFO_QUICK))
> - continue;
> + if (use_oidset &&
> + !oidset_contains(_oid_set, >old_oid)) {
> + /*
> +  * I know this does not exist in the loose form,
> +  * so check if it exists in a non-loose form.
> +  */
> + flags |= OBJECT_INFO_IGNORE_LOOSE;
> + }
>  
> + if (!has_object_file_with_flags(>old_oid, flags))
> + continue;
>   o = parse_object(>old_oid);
>   if (!o)
>   continue;

Re: [PATCH v2] git{,-blame}.el: remove old bitrotting Emacs code

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Junio C. Hamano jotted:

> Martin Ågren  writes:
>
>>> +Emacs's own support for Git got better than what was offered by these
>>> +modules, or was superseded by popular 3rd-party Git modes such as
>>> +Magit.
>>
>> This somehow reads like "Emacs's own support ... was superseded ...".
>> Maybe that's what you mean, but i'm not sure. Perhaps s/, was superseded
>> by/. There are also/.
>
> I think "There are also" is way better.  If we really want to sy
> superseded, perhaps "The features these modules offered were
> superseded by Emacs's own support in VC mode, and popular
> third-party Git modes e.g. Magit" is what we can say.

I sent a v3 a few days ago which addressed this, it's at
20180310184545.16950-1-ava...@gmail.com
(https://public-inbox.org/git/20180310184545.16950-1-ava...@gmail.com/)
but I see that I screwd up the In-Reply-To (I still don't have this
fully automated) so that wasn't part of this thread, sorry.

That changes this wording, among other fixes (noted in the tbdiff).

Re: [PATCH v2] git{,-blame}.el: remove old bitrotting Emacs code

[Junio C Hamano]

Martin Ågren  writes:

>> +Emacs's own support for Git got better than what was offered by these
>> +modules, or was superseded by popular 3rd-party Git modes such as
>> +Magit.
>
> This somehow reads like "Emacs's own support ... was superseded ...".
> Maybe that's what you mean, but i'm not sure. Perhaps s/, was superseded
> by/. There are also/.

I think "There are also" is way better.  If we really want to sy
superseded, perhaps "The features these modules offered were
superseded by Emacs's own support in VC mode, and popular
third-party Git modes e.g. Magit" is what we can say.

Re: Why don't we symlink libexec/git-core/* to bin/git?

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Related to this, I came across this bug report
> https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
> wondering why we're installing N copies of the git binary, presumably
> they're building with NO_INSTALL_HARDLINKS.
> ...
> But is there any reason anyone can think of for why we shouldn't be
> figuring out the relative path and symlinking the two?


There is no fundamental reason not to offer such an "install" method
as an option; unless you count a more philosophical aversion to use
symlinks due to (perceived) additional fragility, that is.

The resulting code may become messier than without, but as long as
it is without the reasonable range for usual price we would pay for
a new "feature", that would be tolerable, I guess.

Re: Bug, git rebase -i does not abort correctly

[Kaartic Sivaraam]

On Friday 26 January 2018 02:54 PM, Duy Nguyen wrote:
> 
> Sort of. It smells bad design to me when a mistake can easily become a
> feature. But with your help, I think I should be able to disable this
> feature on my local build. Thanks.
> 

In case you're still facing this issue, it just struck me recently that
you could have a different alias for 'git rebase -i --onto'. In which
case you could possibly avoid falling prey to the syntax issue ;-)
Something like,

$ git config alias.rio 'rebase -i --onto'



-- 
Kaartic

QUOTE
---

“The most valuable person on any team is the person who makes everyone
else on the team more valuable, not the person who knows the most.”

  - Joel Spolsky



signature.asc
Description: OpenPGP digital signature

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Junio C Hamano]

Johannes Schindelin  writes:

> So essentially, what your cherry-pick'able commits are is a way to store
> what rerere would have stored (i.e. the set of merge conflicts together
> with their resolution)?

If rerere would have stored, I wouldn't have separate band-aid
system on top.  These fix-up commits are usually on parts that do
not get involved in textual conflicts; "rerere" which relies on
having textual conflicts (the "shape" of the text in the conflicted
region is what lets "rerere" index into its database to find the
recorded resolution) wouldn't have stored them and that is
fundamental.

> If so, what tooling do you have to identify quickly what to cherry-pick,
> given merge conflicts?

It exactly is the issue I've been trying to find ideal solution for
quite a while and not successfully.  Here is a sample thread

  https://public-inbox.org/git/xmqqeft3u0u5@gitster.mtv.corp.google.com/#t

and every message I mention "merge-fix" is relevant.

The current band-aid system punts and indexes the merge-fix changes
by merely a branch name.  When refs/merge-fix/X exists, what it
means is "When branch X is merged to an integration branch, it is
likely that the integration branch _already_ has merged an unnamed
topic that causes semantic conflicts and requires this fix-up".
This needs occasional manual adjustment---e.g. when the topic X
turns out to be a lot more stable than the other topic Y that was
causing us trouble with semantic conflicts, I may at some point
reorder the topics and have topic X advance to 'next' before topic Y
does.  And when that happens, when I merge X to 'next', because Y is
not yet in 'next', I shouldn't apply refs/merge-fix/X (often, an
attempt to cherry-pick it on top of a merge of X into 'next' would
fail, which would be a bit of safety, but not always).  What I
should do instead is to rename refs/merge-fix/X to refs/merge-fix/Y
immediately before merging X to 'next', so that the cherry-pick is
not applied.  When rebuilding 'master'->'jch'->'pu' chain, X (now in
'next') will be merged before Y (not in 'next') gets merged, and
when it is Y's turn to be merged, the merge-fix I used to apply when
merging topic X will be applied.

In the ideal world (I think I'm repeating the ideas raised in the
thread quoted), the merge-fix database should be indexed with a pair
of commit object names (e.g. a step in branch X that adds a new
callsite for function frotz() and a step in branch Y that changes
the function signature of frotz()), and teach the system to
cherry-pick refs/merge-fix/A-B to resolve semantic conflicts, when
both commits A and B appears in the integration branch for the first
time.  And make sure these are kept up-to-date across rebasing of
commits A and B.  After rebasing the topics X and Y that contained
the commits A and B, if they became C and D, the system somehow
needs to be able to locate the previous merge-fix that was valid for
A-B pair when C-D pair gets merged.

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Stan Hu]

To be clear, this is how I think we got into this state:

We have worktrees that are created to squash and rebase a branch. They were 
left around inadvertently for one reason or another.

Since we were using git v2.14.3, a git gc would prune dangling objects because 
it never saw that a worktree still had a reference to it. Now, in git v2.15+, 
worktrees are examined, and a git gc won't garbage collect any objects 
referenced by a worktree.

Another words, the failure of git v2.14 to preserve these objects caused 
problems with these stale worktrees with the upgrade to v2.15+.

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Mon, Mar 12, 2018 at 10:30 PM, Junio C Hamano  wrote:
> While I do not think it is a bad idea to add an optional way to write the
> contents of conflicted stages out to separate files, I do not think it is a
> good idea to change what happens to add/add conflict by default.



> If anything, if rename/rename behaves differently by always writing
> the result out to separate files, it is that codepath whose behaviour
> should be fixed to match how add/add conflicts are dealt with.

Cool, thanks for the context.  I'm happy to go down this path, but
there is one question I'd like your opinion on: what if the
intermediate content merges have conflicts themselves?  If that
question isn't clear, let me be more precise...

Let's say A is renamed to C in HEAD, and both sides modified.  We thus
need to do a 3-way content merge of C from HEAD with the two A's.
Let's call the result C1.  Further, let's say that B is renamed to C
in $MERGE_BRANCH, and both sides modified.  Again we need a 3-way
content merge for that other C; let's call the result C2.  If neither
C1 nor C2 have content conflicts, then making rename/rename(2to1) act
like add/add is easy: just do a two-way merge of C1 and C2.  But what
if C1 or C2 have conflicts?  Doing a two-way merge in that case could
result in nested conflict markers.  Is that okay, or would we want to
continue the old behavior of writing C1 and C2 out to separate files
for that special case only?

I'm inclined to special case content conflicts in the intermediate
merges and handle them by writing out to separate files, but I was the
one who suggested sometimes using separate files for add/add
conflicts; I'm curious if perhaps my strong aversion to nested
conflicts is also off base.

Re: Rename of file is causing changes to be lost

[Robert Dailey]

On Tue, Mar 13, 2018 at 11:51 AM, Elijah Newren  wrote:
> Anyway, I recorded this at
> https://bugs.chromium.org/p/git/issues/detail?id=11.  Sorry I don't
> have a workaround, but I'll try to eventually get back to this and fix
> it.

Thank you for taking the time to verify this for me. I will keep an
eye on the issue you linked!

[GSoC][Project Idea] Multipart and resumable download while using git clone or pull

[Varun Garg]

Hi,

My name is Varun Garg and am a student in Software Engineering. During 2017 I 
participated in GSoC at Eclipse Foundation [1] and still contribute 
occasionally [2]. I have also submitted a micro project here last year[3].

I would like to add functionality of "Multipart and resumable download while 
using git clone or pull" in git.

Motivation:
The motivation behind this idea came to me when I was downloading linux kernel. 
Even with shallow clone (--depth=1), the number of objects was too large. Many 
times we need access to older commits. It is also difficult to update shallow 
cloned repos [4]. And ssh option is not available behind many firewalls.

Downloading objects incrementally is too slow and we can speed this process by 
downloading them in parallel (multi part) and adding a resume option will also 
be lifesaver.

I am looking forward to take this project.

[1] https://summerofcode.withgoogle.com/archive/2017/projects/5762172684599296/
[2] https://github.com/eclipse/omr/pulls?q=author%3AVarun-garg
[3] 
https://public-inbox.org/git/cy1pr19mb01289dca6c08cdc9f07c6d7df2...@cy1pr19mb0128.namprd19.prod.outlook.com/
[4] 
https://stackoverflow.com/questions/41075972/how-to-update-a-git-shallow-clone

With Best Regards,
Varun Garg 

Re: [PATCH v3] fetch-pack.c: use oidset to check existence of loose object

[Junio C Hamano]

Takuto Ikuta  writes:

>> During fetch, everything_local() tries to mark common part by
>> walking the refs the other side advertised upon the first contact,
>> so it is correct that the number of checks is not reduced in a fetch
>> that does not fetch many refs, but the number of remote-tracking refs
>> you have has no effect, so I doubt such a rephrasing would make the
>> description more understandable.  "When fetching from a repository
>> with large number of refs" is probably what you want to say, no?
>
> For refs existing in local repository, everything_local looks to be able to 
> find
> corresponding object from packed or loose objects. And if it exists,
> I think, cost of lstat(2) is relatively smaller than other operations.
> But for remote refs, everything_local fails to find it from packed
> object (this check is fast)
> and it tries to find loose object by using lstat(2), and this fails and slow.
> My patch is to skip this lstat(2) to non-existing ref objects for repositories
> having large number of remote refs.

This still does not make sense to me, and I suspect that I am
misreading you.  In what sense are you using the word "repository"
and "remote refs"?

Imagine this situation.  I have a local repository A, I fetch from a
remote repository B but in my repository A, I do *not* use
remote-tracking refs to remember what the last values of refs at
repository B.  Now when I try to fetch a single ref from B into A,
many refs B advertises point at objects A has never heard about, and
that triggers many lstat(2) that yields ENOENT that is slow.  Your
patch is to optimize this so that we learn these objects do not
exist locally without running many lstat(2) to objects and helps
repositories (like my repository A) when fetching from a repository
with large number of refs (like the repository B).  It does not
matter how many "remote refs" receiving repository (e.g. my A) has,
and it does not matter how many "remote refs" sending repository
(e.g. my B) has---whether it is refs/remotes/origin/foo
(i.e. "remote") or refs/heads/foo (i.e. "local"), a ref at B that
points at an object that is missing at A will cause the same
lstat(2) at A without your change.

That is why I think "When fetching from a repository with large
number of refs" is what you meant, not "fetching into a repository
with large number of remote refs" nor "fetching from a repository
with large number of remote refs".

Thanks.

Re: [GSoC] [PATCH] travis-ci: added clang static analysis

[Siddhartha Mishra]

I'm extremely sorry, I forgot to attach my travis-ci job build with my
previous message. Here it is :
https://travis-ci.org/SiddharthaMishra/git/jobs/349478746#L1204.

I also apologize for not trimming the email properly in the previous message.

Sorry for the inconvenience,
Siddhartha

Re: [GSoC] [PATCH] travis-ci: added clang static analysis

[Siddhartha Mishra]

On Mon, Mar 12, 2018 at 3:49 PM, Lars Schneider
 wrote:
> Hi,
>
> That looks interesting but I agree with Dscho that we should not limit
> this to master/maint.
>
> I assume you did run this on TravisCI already? Can you share a link?
> I assume you did find errors? Can we fix them or are there too many?
> If there are existing errors, how do we define a "successful" build?
>
> Thanks for working on this,
> Lars
>

Thanks for the reply,

I assume there will be false positives in the code which we can't fix
by making small modifications to the code as recommended in the FAQ
(https://clang-analyzer.llvm.org/faq.html). According to the FAQ,
there is no solid mechanism for suppressing a specific warning, so are
options are limited. Some of the things which might help reduce the
noise are:

1) To add specific tags in our source code to tell the analyzer to
ignore the code. This is probably a bad idea since it is intrusive and
forces changes to the actual source code which only affect one task.

2) Count the number of bugs in the previous pushed build and fail the
build if the number of bugs increases. It doesn't help remove the
noise from the error log but it does tell you if you've added more
bugs. However if you add a bug and remove one, it'll pass the job and
might mislead you into thinking that the code is correct.

3) Write a script to check the diff of the error log from that of the
previous pushed build(ignoring the line numbers). I haven't thought
about how exactly it would be implemented so I'm not commenting on it.

Is there a better solution that I'm missing or should I try coming up
with a script to come up the diff?

Thanks for the time,
Siddhartha

On Mon, Mar 12, 2018 at 3:49 PM, Lars Schneider
 wrote:
> Hi,
>
> That looks interesting but I agree with Dscho that we should not limit
> this to master/maint.
>
> I assume you did run this on TravisCI already? Can you share a link?
> I assume you did find errors? Can we fix them or are there too many?
> If there are existing errors, how do we define a "successful" build?
>
> Thanks for working on this,
> Lars
>
>> On 05 Mar 2018, at 21:04, SiddharthaMishra  wrote:
>>
>> Added a job to run clang static code analysis on the master and maint branch
>>
>> Signed-off-by: SiddharthaMishra 
>> ---
>> .travis.yml   | 17 -
>> ci/run-static-analysis.sh |  9 -
>> 2 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 4684b3f4f..9b891d182 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -48,7 +48,7 @@ matrix:
>>   before_install:
>>   before_script:
>>   script: ci/run-linux32-docker.sh
>> -- env: jobname=StaticAnalysis
>> +- env: jobname=CocciStaticAnalysis
>>   os: linux
>>   compiler:
>>   addons:
>> @@ -59,6 +59,21 @@ matrix:
>>   before_script:
>>   script: ci/run-static-analysis.sh
>>   after_failure:
>> +- if: branch IN (master, maint)
>> +  env: jobname=ClangStaticAnalysis
>> +  os: linux
>> +  compiler:
>> +  add_ons:
>> +apt:
>> +  sources:
>> +  - ubuntu-toolchain-r-test
>> +  - llvm-toolchain-trusty
>> +  packages:
>> +  - clang
>> +  before_install:
>> +  before_script:
>> +  script: ci/run-static-analysis.sh
>> +  after_failure:
>> - env: jobname=Documentation
>>   os: linux
>>   compiler:
>> diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
>> index fe4ee4e06..6ae032f54 100755
>> --- a/ci/run-static-analysis.sh
>> +++ b/ci/run-static-analysis.sh
>> @@ -5,6 +5,13 @@
>>
>> . ${0%/*}/lib-travisci.sh
>>
>> -make coccicheck
>> +case "$jobname" in
>> +ClangStaticAnalysis)
>> + scan-build -analyze-headers --status-bugs make
>> + ;;
>> +CocciStaticAnalysis)
>> + make coccicheck
>> + ;;
>> +esac
>>
>> save_good_tree
>> --
>> 2.16.2.248.ge2408a6f7.dirty
>>
>

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Mon, Mar 12, 2018 at 5:38 PM, Elijah Newren  wrote:
> I feel the analogy to merging binary files breaks down here in more
> than one way:

Actually, after mulling this over, I'm going to retract the "more
than" piece of this sentence.  I'm trying to figure out how to retract
more, but have only figured out one piece.  In particular...

> 1)
>
> Merging binary files is more complex than you suggest here.  In
> particular, when creating a virtual merge base, the resolution chosen
> is not the version of the file from HEAD, but the version of the file
> from the merge base.  Nasty problems would result otherwise for the
> recursive case.
>
> If we tried to match how merging binary files behaved, we run into the
> problem that the colliding file conflict case has no common version of
> the file from a merge base.  So the same strategy just doesn't apply.
> The closest would be outright deleting both versions of the file for
> the virtual merge base and punting to the outer merge to deal with it.
> That might be okay, but seems really odd to me.  I feel like it'd
> unnecessarily increase the number of conflicts users will see, though
> maybe it wouldn't be horrible if this was only done when the files
> were considered dissimilar anyway.

Thinking about this more, it really isn't that weird at all.  The code
is already set up to use null_oid as the "original" version when
creating a virtual merge base, going back to the content from a
recursive merge base is a strategy used in multiple places in
recursive cases, and null is precisely the version from the recursive
merge base.  If two added files differ wildly, I don't think using
null would increase the number of conflicts appreciably, if at all.
So, the analogy to merging binary files holds just fine from this
angle.

So, if we could figure out how to handle the higher numbers of paths
for e.g. the rename/rename cases, then perhaps this is a strategy that
could work.

>> Interesting.  I would be tempted to resolve this inconsistency the
>> other way: by doing a half-hearted two-way merge (e.g. by picking one
>> of the two versions of the colliding file) and marking the path as
>> conflicted in the index.  That way it's more similar to edit/edit,
>> too.
>
> Your proposal is underspecified; there are more complicated cases
> where your wording could have different meanings.
>

> My question for your counter-proposal:
> Do you record C1 or C2 as C?  Or do you record the version of C from
> HEAD as C?  And what do you pick when creating a virtual merge base?
>
> Problems I see regardless of the choice in your counter-proposal:
>   * If you choose C from HEAD, you are ignoring 3 other versions of
> "C" (as opposed to just one other version when merging a binary file);
> will the user recognize that and know how to look at all four
> versions?
>   * If you choose C1 or C2, the user will see conflict markers; will
> the user recognize that this is only a _subset_ of the conflicts, and
> that there's a lot of content that was completely ignored?

>   * There is no "merge base of C1 and C2" to use for the virtual merge
> base.  What will you use?

For this last question, the answer is "null", and it works just fine.
I don't yet have good answers to the other questions, though.  If
someone else does, I'd love to hear it.


Oh, and by the way Jonathan, thanks very much for your feedback and
alternative ideas for consideration.  You gave me more angles to try
to think about this problem from.

Re: Opinions on changing add/add conflict resolution?

[Elijah Newren]

On Tue, Mar 13, 2018 at 2:59 AM, Ævar Arnfjörð Bjarmason
 wrote:
> On Tue, Mar 13 2018, Elijah Newren jotted:

>> However, I'm far more concerned with the three collision conflict types
>> having consistent behavior than I am with changing add/add conflict
>> handling.  And if your two concerns or Jonathan's concern would prevent you
>> from wanting to use the new merge strategy (and possibly prevent it from
>> becoming the default in the future), I'd much rather just modify rename/add
>> and rename/rename to behave like add/add.  Would that be more to your
>> liking?
>
> I don't really object to these changes, I don't know enough about this
> area, I skimmed your patches and 90% of it is over my head (although I
> could keep digging...).
>
> I'm just chiming in because it seems to me from upthread that you're
> purely focusing on the use-case of the user of git who's using git at
> the abstraction of using a dumb editor that doesn't do anything to help
> them with conflict markers to resolve conflicts.

Yeah, that's totally why I started this separate thread; I was worried
the original would push away folks who weren't familiar enough with
merge-recursive or were just overwhelmed by all the different changes
and rationale, but I really wanted to get feedback on at least the
piece that people were likely to hit in practice and would understand.
Thanks for doing so; your, and Jonathan's, and Junio's comments have
provided some good context.

> Also another caveat: Since these are new side-files what happens when
> you're in a conflict and run `git clean -dxf`, are these new files wiped
> away, or are they somehow known to the index?

A git clean would wipe them out...and if that scares you (and I can
totally see how it might), then rest assured that the situation is a
whole lot worse: this isn't limited to rename/add and
rename/rename(2to1) conflicts.  There are several paths in
merge-recursive.c that call unique_path() to get a temporary filename
that is in no way tracked in the index but which is used for storing
content relevant to the merge.  These include directory/file
conflicts, untracked files being in the way of putting a renamed file
where it belongs, untracked files being in the way of a modify/delete,
untracked files being in the way of simple adds on the other side of
history, rename/rename(1to2)/add/add, and maybe others.  In all cases,
a git clean is going to wipe out the files that were written to
different temporary locations.

My rewrite I'm trying to find time to work on would get rid of the
code paths involving untracked files being in the way of other stuff,
but would do nothing for the other cases.  I would love to get rid of
all of them, but don't have a clue how to do so.

[PATCH] sha1_file: restore OBJECT_INFO_QUICK functionality

[Jonathan Tan]

Support for the OBJECT_INFO_QUICK flag in sha1_object_info_extended()
was added in commit dfdd4afcf9 ("sha1_file: teach
sha1_object_info_extended more flags", 2017-06-26) in order to support
commit e83e71c5e1 ("sha1_file: refactor has_sha1_file_with_flags",
2017-06-26), but it was inadvertently removed in commit 8b4c0103a9
("sha1_file: support lazily fetching missing objects", 2017-12-08).

Restore this functionality.

Signed-off-by: Jonathan Tan 
---
> Hmm, OBJECT_INFO_QUICK optimization was added in dfdd4afc
> ("sha1_file: teach sha1_object_info_extended more flags",
> 2017-06-21), but since 8b4c0103 ("sha1_file: support lazily fetching
> missing objects", 2017-12-08) it appears that passing
> OBJECT_INFO_QUICK down the codepath does not do anything
> interesting.  Jonathan (cc'ed), are all remaining hits from "git
> grep OBJECT_INFO_QUICK" all dead no-ops these days?

They are, but they are not supposed to be. Here is a patch correcting
that.
---
 sha1_file.c | 8 +---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 1b94f39c4..cc0f43ea8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1267,9 +1267,11 @@ int sha1_object_info_extended(const unsigned char *sha1, 
struct object_info *oi,
return 0;
 
/* Not a loose object; someone else may have just packed it. */
-   reprepare_packed_git();
-   if (find_pack_entry(real, ))
-   break;
+   if (!(flags & OBJECT_INFO_QUICK)) {
+   reprepare_packed_git();
+   if (find_pack_entry(real, ))
+   break;
+   }
 
/* Check if it is a missing object */
if (fetch_if_missing && repository_format_partial_clone &&
-- 
2.16.2.660.g709887971b-goog

Re: Rename of file is causing changes to be lost

[Elijah Newren]

On Thu, Mar 8, 2018 at 8:01 AM, Robert Dailey  wrote:
> I'm on Windows and core.ignorecase is set to 'true' when I clone/init
> a repository. I've got a branch where I started making changes to a
> file AND renamed it only to change its case. The changes I've made
> were significant enough that git no longer detects a rename, instead
> the files show up as "D" and "A" in git status (deleted then added).
> To correct this, I do an interactive rebase to add an additional
> commit before the first one to rename the file without changing it,
> and *then* allow the second commit to change the file. The goal is
> that rebase should detect the rename and automatically move the
> changes in the (now) second commit to the newly named file. Here's a
> MCVE (treat this as a script):
>
> #/bin/bash
> git init testgitrepo
> cd testgitrepo/
> git config core.ignorecase true # This is set by Windows for me, but
> hopefully will allow this to repro on linux. Didn't test linux though.
> echo "first change" > foo.txt
> git add . && git commit -m 'first change'
> git checkout -b topic
> echo "second change" > foo.txt
> git mv foo.txt FOO.txt
> git add . && git commit -m 'second change'
> git rebase -i master # Move line 1 to line 2, and put "x false" in line 1
> git mv foo.txt FOO.txt && git commit -m 'rename foo'
> git rebase --continue
> git mergetool
>
> After the rebase continue, you will get a conflict like so:
>
> error: could not apply 527d208... second change
>
> When you have resolved this problem, run "git rebase --continue".
> If you prefer to skip this patch, run "git rebase --skip" instead.
> To check out the original branch and stop rebasing, run "git rebase --abort".
>
> Could not apply 527d208... second change
> CONFLICT (rename/delete): foo.txt deleted in 527d208... second change
> and renamed to FOO.txt in HEAD. Version HEAD of FOO.txt left in tree.
>
> The last command, `git mergetool` runs, giving you the option to pick
> the Created (left) or Deleted (right) version of the file:
>
> Left: The file is created, but selecting this erases the changes from
> the "added" version on the remote (which is topic). Basically the
> rename of only case confused git, and we lost the changes on the
> remote version of the file
> Right: File is deleted. Changes are still lost.
>
> The ideal outcome is that the changes from the "added" version of the
> file in the 2nd commit get carried over to the "renamed" version of
> the file, which when you compare the two are named exactly the same
> after the 1st commit is introduced. How can I solve this issue?

Cool, thanks for the testcase.  I don't have a good workaround for
you, but this is clearly a bug in the merge-recursive logic in git.  I
guess it's what might be called a rename/add/delete conflict, which
git just doesn't handle.  Your testcase triggers the bug just fine on
linux, though you can trigger the exact same bug without case
sensitivity using a slightly different setup (and no need for an
interactive rebase):

--
git init foobar
cd foobar
echo "original file" >foo
git add foo
git commit -m "original"
git branch L
git branch R

git checkout L
git rm foo
echo "different file" >bar
git add bar
git commit -m "Remove foo, add bar"

git checkout R
git mv foo bar
git commit -m "rename foo to bar"

git merge L
---

git has code to handle rename/delete conflicts and rename/add
conflicts, but since one side of history BOTH deleted foo and added an
unrelated bar, that means both types of changes are relevant to the
same path (bar) -- essentially, a rename/delete/add conflict.  Sadly,
git only goes down a codepath that handles one of those two (the
rename/delete), and incorrectly throws away the separate add:

-
$ git ls-files -s
100644 78fa0f415ae2bdb5c0182c067eacaaf843699b39 2bar

$ git ls-tree -r master
100644 blob 78fa0f415ae2bdb5c0182c067eacaaf843699b39foo
$ git ls-tree -r L
100644 blob f286e5cdd97ac6895438ea4548638bb98ac9bd6bbar
$ git ls-tree -r R
100644 blob 78fa0f415ae2bdb5c0182c067eacaaf843699b39bar
-

But the problem is actually a bit bigger than shown here; there are
higher order corner cases here too.  I realized in the past that e.g.
rename/rename(1to2) could also have rename/add conflicts for each
rename (thus making a rename/rename/add/add conflict possible), but I
also felt there were probably some other bad interactions out there.
I figured they were likely theoretical only, so I didn't bother
investigating.  But, combining your example with that other one, we
should also be able to come up with a
rename/rename/add/add/delete/delete conflict.  I wonder if there are
others...

Anyway, I recorded this at
https://bugs.chromium.org/p/git/issues/detail?id=11.  Sorry I don't
have a workaround, but I'll try to eventually get back to this and fix
it.

Re: [PATCH v4 04/35] upload-pack: convert to a builtin

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:21 -0800
Brandon Williams  wrote:

> In order to allow for code sharing with the server-side of fetch in
> protocol-v2 convert upload-pack to be a builtin.
> 
> Signed-off-by: Brandon Williams 

I suggested updating the commit message in my previous review [1], but I
understand that my comment might have been lost in the ensuing long
discussion.

[1] 
https://public-inbox.org/git/20180221134422.2386e1aca39fe67323559...@google.com/

Re: [PATCH v4 35/35] remote-curl: don't request v2 when pushing

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:52 -0800
Brandon Williams  wrote:

> In order to be able to ship protocol v2 with only supporting fetch, we
> need clients to not issue a request to use protocol v2 when pushing
> (since the client currently doesn't know how to push using protocol v2).
> This allows a client to have protocol v2 configured in
> `protocol.version` and take advantage of using v2 for fetch and falling
> back to using v0 when pushing while v2 for push is being designed.
> 
> We could run into issues if we didn't fall back to protocol v2 when
> pushing right now.  This is because currently a server will ignore a request 
> to
> use v2 when contacting the 'receive-pack' endpoint and fall back to
> using v0, but when push v2 is rolled out to servers, the 'receive-pack'
> endpoint will start responding using v2.  So we don't want to get into a
> state where a client is requesting to push with v2 before they actually
> know how to push using v2.
> 
> Signed-off-by: Brandon Williams 

I noticed that my review comments have been addressed, so:

Reviewed-by: Jonathan Tan 

Re: [PATCH v4 31/35] http: allow providing extra headers for http requests

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:48 -0800
Brandon Williams  wrote:

> Add a way for callers to request that extra headers be included when
> making http requests.
> 
> Signed-off-by: Brandon Williams 

Likewise,

Reviewed-by: Jonathan Tan 

Re: [PATCH v4 29/35] remote-curl: create copy of the service name

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:46 -0800
Brandon Williams  wrote:

> Make a copy of the service name being requested instead of relying on
> the buffer pointed to by the passed in 'const char *' to remain
> unchanged.
> 
> Currently, all service names are string constants, but a subsequent
> patch will introduce service names from external sources.
> 
> Signed-off-by: Brandon Williams 

Once again,

Reviewed-by: Jonathan Tan 

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Junio C Hamano]

Phillip Wood  writes:

> On 08/03/18 17:53, Junio C Hamano wrote:
>> Phillip Wood  writes:
>> 
>>> and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
>>> to mean everything from 'n' to the last line though.
>> 
>> Thanks for double checking.  It would be a better endgame to follow
>> up with an update to existing "range selection" code to also support
>> "n-", if you go that route.
>> 
> I'm afraid I'm not sure exactly what you're suggesting. At the moment
> the range selection code is in the first patch and supports incomplete
> ranges. Are you suggesting that support for incomplete ranges should be
> in a separate patch or have I misunderstood?

My observation of the situation behind my reasoning is:

 - There is an existing UI that uses "-X" to mean "exclude what
   matches X" and that was the reason why you decided to follow suit
   instead of using "^X" for inversion of X.

 - Such an existing UI would not have used "-X" to mean "the first
   possible choice thru X".  You will lose that from your new thing
   and you accepted that.

 - It is likely (I did not check, though) that the existing UI would
   not have used "Y-" to mean "starting from Y all the possible
   choices thru to the end", but that is merely for symmetry with
   the lack (inability to use) of "-X".  There is no fundamental
   reason why "Y-" cannot mean that, and you are tempted to allow do
   so in your new thing for the same reason.

So if we are going to have "N-" to mean "everything from N to the
last line", then the same "Starting at N to the end of the all the
possible choices" should be allowed in the existing UI (i.e. the one
that forced you to give up "^X" for the sake of consistency) for the
same consistency reasons, no?

For that, if you want to keep the "n-" you did in your first patch,
the most logical thing is to have a preparatory enhancement to teach
"N-" to list_and_choose(), and then build your series on top.  Or
you can do without such a change to list_and_choose() in your series,
in which case, you drop "n-" support and then at the very end after
the series settles, add "n-" support to the new code in this series
and to list_and_choose() at the same time in a follow-up patch.

Re: [PATCH v4 27/35] transport-helper: introduce stateless-connect

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:44 -0800
Brandon Williams  wrote:

> +'stateless-connect'::
> + Experimental; for internal use only.
> + Can attempt to connect to a remote server for communication
> + using git's wire-protocol version 2.  This establishes a
> + stateless, half-duplex connection.
> ++
> +Supported commands: 'stateless-connect'.
> +
>  'push'::
>   Can discover remote refs and push local commits and the
>   history leading up to them to new or existing remote refs.
> @@ -136,6 +144,14 @@ Capabilities for Fetching
>  +
>  Supported commands: 'connect'.
>  
> +'stateless-connect'::
> + Experimental; for internal use only.
> + Can attempt to connect to a remote server for communication
> + using git's wire-protocol version 2.  This establishes a
> + stateless, half-duplex connection.
> ++
> +Supported commands: 'stateless-connect'.

I don't think we should use the term "half-duplex" - from a search, it
means that both parties can use the wire but not simultaneously, which
is not strictly true. Might be better to just say "see the documentation
for the stateless-connect command for more information".

> +'stateless-connect' ::
> + Experimental; for internal use only.
> + Connects to the given remote service for communication using
> + git's wire-protocol version 2.  This establishes a stateless,
> + half-duplex connection.  Valid replies to this command are empty
> + line (connection established), 'fallback' (no smart transport
> + support, fall back to dumb transports) and just exiting with
> + error message printed (can't connect, don't bother trying to
> + fall back).  After line feed terminating the positive (empty)
> + response, the output of the service starts.  Messages (both
> + request and response) must be terminated with a single flush
> + packet, allowing the remote helper to properly act as a proxy.
> + After the connection ends, the remote helper exits.
> ++
> +Supported if the helper has the "stateless-connect" capability.

I'm not sure of the relevance of "allowing the remote helper to properly
act as a proxy" - this scheme does make it easier to implement proxies,
not for any party to start acting as one instead. I would write that
part as:

Messages (both request and response) must consist of zero or more
PKT-LINEs, terminating in a flush packet. The client must not expect
the server to store any state in between request-response pairs.

(This covers the so-called "half-duplex" part and the "stateless" part.)

Re: [PATCH v4 20/35] upload-pack: introduce fetch server command

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:37 -0800
Brandon Williams  wrote:

> +output = *section
> +section = (acknowledgments | packfile)
> +   (flush-pkt | delim-pkt)
> +
> +acknowledgments = PKT-LINE("acknowledgments" LF)
> +   (nak | *ack)
> +   (ready)
> +ready = PKT-LINE("ready" LF)
> +nak = PKT-LINE("NAK" LF)
> +ack = PKT-LINE("ACK" SP obj-id LF)
> +
> +packfile = PKT-LINE("packfile" LF)
> +[PACKFILE]

I should have noticed this earlier, but "PACKFILE" is not defined anywhere -
it's probably better written as:

*PKT-LINE(%x01-03 *%x00-ff)"

or something like that.

> +acknowledgments section
> + * Always begins with the section header "acknowledgments"
> +
> + * The server will respond with "NAK" if none of the object ids sent
> +   as have lines were common.
> +
> + * The server will respond with "ACK obj-id" for all of the
> +   object ids sent as have lines which are common.
> +
> + * A response cannot have both "ACK" lines as well as a "NAK"
> +   line.
> +
> + * The server will respond with a "ready" line indicating that
> +   the server has found an acceptable common base and is ready to
> +   make and send a packfile (which will be found in the packfile
> +   section of the same response)
> +
> + * If the client determines that it is finished with negotiations
> +   by sending a "done" line, the acknowledgments sections MUST be
> +   omitted from the server's response.
> +
> + * If the server has found a suitable cut point and has decided
> +   to send a "ready" line, then the server can decide to (as an
> +   optimization) omit any "ACK" lines it would have sent during
> +   its response.  This is because the server will have already
> +   determined the objects it plans to send to the client and no
> +   further negotiation is needed.
> +
> +
> +packfile section
> + * Always begins with the section header "packfile"
> +
> + * The transmission of the packfile begins immediately after the
> +   section header
> +
> + * The data transfer of the packfile is always multiplexed, using
> +   the same semantics of the 'side-band-64k' capability from
> +   protocol version 1.  This means that each packet, during the
> +   packfile data stream, is made up of a leading 4-byte pkt-line
> +   length (typical of the pkt-line format), followed by a 1-byte
> +   stream code, followed by the actual data.
> +
> +   The stream code can be one of:
> + 1 - pack data
> + 2 - progress messages
> + 3 - fatal error message just before stream aborts
> +
> + * This section is only included if the client has sent 'want'
> +   lines in its request and either requested that no more
> +   negotiation be done by sending 'done' or if the server has
> +   decided it has found a sufficient cut point to produce a
> +   packfile.

For both the sections, I think that the conditions for
inclusion/non-inclusion ("This section is only included if...") should
be the first point.

> +static void upload_pack_data_init(struct upload_pack_data *data)
> +{
> + struct object_array wants = OBJECT_ARRAY_INIT;
> + struct oid_array haves = OID_ARRAY_INIT;
> +
> + memset(data, 0, sizeof(*data));
> + data->wants = wants;
> + data->haves = haves;
> +}

Any reason to use a initializer function instead of a static literal?

Re: [RFC] Rebasing merges: a jorney to the ultimate solution (Road Clear)

[Sergey Organov]

Hi Phillip,

Phillip Wood  writes:

[...]

> Hi Sergey, I've been following this discussion from the sidelines,
> though I haven't had time to study all the posts in this thread in
> detail. I wonder if it would be helpful to think of rebasing a merge as
> merging the changes in the parents due to the rebase back into the
> original merge. So for a merge M with parents A B C that are rebased to
> A' B' C' the rebased merge M' would be constructed by (ignoring shell
> quoting issues)
>
> git checkout --detach M
> git merge-recursive A -- M A'
> tree=$(git write-tree)
> git merge-recursive B -- $tree B'
> tree=$(git write-tree)
> git merge-recursive C -- $tree C'
> tree=$(git write-tree)
> M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB' -pC')

I wonder if it's OK to exchange the order of heads in the first merge
(also dropped C for brevity):

git checkout --detach A'
git merge-recursive A -- A' M
tree=$(git write-tree)
git merge-recursive B -- $tree B'
tree=$(git write-tree)
M'=$(git log --pretty=%B -1 M | git commit-tree -pA' -pB')

If so, don't the first 2 lines now read: "rebase (first parent of) M on
top of A'"?

If so, then it could be implemented so that it reduces back to regular
rebase of non-merges when applied to a single-parent commit, similar to
the method in the RFC, striking out one of advantages of the RFC.

-- Sergey

Re: [PATCH v4 16/35] transport: convert transport_get_remote_refs to take a list of ref patterns

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:33 -0800
Brandon Williams  wrote:

> Convert 'transport_get_remote_refs()' to optionally take a list of ref
> patterns.
> 
> Signed-off-by: Brandon Williams 

[snip]

> -const struct ref *transport_get_remote_refs(struct transport *transport);
> +/*
> + * Retrieve refs from a remote.
> + *
> + * Optionally a list of ref patterns can be provided which can be sent to the
> + * server (when communicating using protocol v2) to enable it to limit the 
> ref
> + * advertisement.  Since ref filtering is done on the server's end (and only
> + * when using protocol v2), this can return refs which don't match the 
> provided
> + * ref_patterns.
> + */
> +const struct ref *transport_get_remote_refs(struct transport *transport,
> + const struct argv_array 
> *ref_patterns);

Thanks for adding the documentation, but I think this should also go
into the commit message. For example:

Teach transport_get_remote_refs() to accept a list of ref patterns,
which will be sent to the server for use in filtering when using
protocol v2. (This list will be ignored when not using protocol v2.)

Re: [PATCH v4 08/35] connect: discover protocol version outside of get_remote_heads

[Jonathan Tan]

On Wed, 28 Feb 2018 15:22:25 -0800
Brandon Williams  wrote:

> In order to prepare for the addition of protocol_v2 push the protocol
> version discovery outside of 'get_remote_heads()'.  This will allow for
> keeping the logic for processing the reference advertisement for
> protocol_v1 and protocol_v0 separate from the logic for protocol_v2.
> 
> Signed-off-by: Brandon Williams 

I had one issue in version 3 which turns out to not be one, and this
patch is unchanged from version 3, so:

Reviewed-by: Jonathan Tan 

> ---
>  builtin/fetch-pack.c | 16 +++-
>  builtin/send-pack.c  | 17 +++--
>  connect.c| 27 ++-
>  connect.h|  3 +++
>  remote-curl.c| 20 ++--
>  remote.h |  5 +++--
>  transport.c  | 24 +++-
>  7 files changed, 83 insertions(+), 29 deletions(-)
> 
> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index 366b9d13f..85d4faf76 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -4,6 +4,7 @@
>  #include "remote.h"
>  #include "connect.h"
>  #include "sha1-array.h"
> +#include "protocol.h"
>  
>  static const char fetch_pack_usage[] =
>  "git fetch-pack [--all] [--stdin] [--quiet | -q] [--keep | -k] [--thin] "
> @@ -52,6 +53,7 @@ int cmd_fetch_pack(int argc, const char **argv, const char 
> *prefix)
>   struct fetch_pack_args args;
>   struct oid_array shallow = OID_ARRAY_INIT;
>   struct string_list deepen_not = STRING_LIST_INIT_DUP;
> + struct packet_reader reader;
>  
>   packet_trace_identity("fetch-pack");
>  
> @@ -193,7 +195,19 @@ int cmd_fetch_pack(int argc, const char **argv, const 
> char *prefix)
>   if (!conn)
>   return args.diag_url ? 0 : 1;
>   }
> - get_remote_heads(fd[0], NULL, 0, , 0, NULL, );
> +
> + packet_reader_init(, fd[0], NULL, 0,
> +PACKET_READ_CHOMP_NEWLINE |
> +PACKET_READ_GENTLE_ON_EOF);
> +
> + switch (discover_version()) {
> + case protocol_v1:
> + case protocol_v0:
> + get_remote_heads(, , 0, NULL, );
> + break;
> + case protocol_unknown_version:
> + BUG("unknown protocol version");
> + }
>  
>   ref = fetch_pack(, fd, conn, ref, dest, sought, nr_sought,
>, pack_lockfile_ptr);
> diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> index fc4f0bb5f..83cb125a6 100644
> --- a/builtin/send-pack.c
> +++ b/builtin/send-pack.c
> @@ -14,6 +14,7 @@
>  #include "sha1-array.h"
>  #include "gpg-interface.h"
>  #include "gettext.h"
> +#include "protocol.h"
>  
>  static const char * const send_pack_usage[] = {
>   N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
> @@ -154,6 +155,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
> *prefix)
>   int progress = -1;
>   int from_stdin = 0;
>   struct push_cas_option cas = {0};
> + struct packet_reader reader;
>  
>   struct option options[] = {
>   OPT__VERBOSITY(),
> @@ -256,8 +258,19 @@ int cmd_send_pack(int argc, const char **argv, const 
> char *prefix)
>   args.verbose ? CONNECT_VERBOSE : 0);
>   }
>  
> - get_remote_heads(fd[0], NULL, 0, _refs, REF_NORMAL,
> -  _have, );
> + packet_reader_init(, fd[0], NULL, 0,
> +PACKET_READ_CHOMP_NEWLINE |
> +PACKET_READ_GENTLE_ON_EOF);
> +
> + switch (discover_version()) {
> + case protocol_v1:
> + case protocol_v0:
> + get_remote_heads(, _refs, REF_NORMAL,
> +  _have, );
> + break;
> + case protocol_unknown_version:
> + BUG("unknown protocol version");
> + }
>  
>   transport_verify_remote_names(nr_refspecs, refspecs);
>  
> diff --git a/connect.c b/connect.c
> index c82c90b7c..0b111e62d 100644
> --- a/connect.c
> +++ b/connect.c
> @@ -62,7 +62,7 @@ static void die_initial_contact(int unexpected)
> "and the repository exists."));
>  }
>  
> -static enum protocol_version discover_version(struct packet_reader *reader)
> +enum protocol_version discover_version(struct packet_reader *reader)
>  {
>   enum protocol_version version = protocol_unknown_version;
>  
> @@ -233,7 +233,7 @@ enum get_remote_heads_state {
>  /*
>   * Read all the refs from the other end
>   */
> -struct ref **get_remote_heads(int in, char *src_buf, size_t src_len,
> +struct ref **get_remote_heads(struct packet_reader *reader,
> struct ref **list, unsigned int flags,
> struct oid_array *extra_have,
> struct oid_array *shallow_points)
> @@ -241,24 +241,17 @@ struct ref **get_remote_heads(int in, char *src_buf, 
> size_t src_len,
>   struct ref 

Re: [GSoC][PATCH] git-ci: use pylint to analyze the git-p4 code

[Luke Diamand]

On 13 March 2018 at 03:36, Viet Hung Tran  wrote:
> Hello Mr. Schneider,
>
> Here is my link for the passed CI build I tested on my fork:
> https://travis-ci.org/VietHTran/git/builds/35210
>
> In order to test .travis.yml configuration alone, I used a sample python
> script call "test.py" that is already successfully tested on my computer
> instead of using the git-p4.py like in the patch I submitted since the
> git-p4.py file still reports a lot of errors when running pylint.
>
> It is possible to fix all the git-p4.py errors. However, I think it would
> be best to fix each error separately with multiple commits (each should
> fix a specific problem such as indentication, variable naming, etc.)
> since there are a lot of changes needed to be made in the codes. Since the
> microproject requires that I submit one patch, I decided to submit a patch
> that includes changes in .travis.yml only. If you like, I can also submit the
> patches addressing changes on the git-p4.py that fix the pylint errors.

You can turn down the amount of messages it spews out, at which point
it actually becomes quite useful.

I had a quick go with the list of enabled checkers found here:

   https://github.com/ClusterHQ/flocker/blob/master/.pylintrc

and it then gives about 40 warnings, all of which look like they could
do with fixing.

I think fixing them in a few patches would be OK, rather than
submitting 40 separate patches.

Quite a lot of the warnings are about the use of "file" - I've got a
change I'm working on which fixes some of those already.

Luke


>
> Thank you for feedback,
>
> Viet
>
> Signed-off-by: Viet Hung Tran 
> ---
>  .travis.yml | 10 ++
>  1 file changed, 10 insertions(+)
>
> diff --git a/.travis.yml b/.travis.yml
> index 5f5ee4f3b..581d75319 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -46,6 +46,16 @@ matrix:
>  - docker
>before_install:
>script: ci/run-linux32-docker.sh
> +- env: jobname=Pylint
> +  compiler:
> +  addons:
> +apt:
> +  packages:
> +  - python
> +  before_install:
> +  install: pip install --user pylint
> +  script: pylint git-p4.py
> +  after_failure:
>  - env: jobname=StaticAnalysis
>os: linux
>compiler:
> --
> 2.16.2.440.gc6284da
>

Re: Rename of file is causing changes to be lost

[Robert Dailey]

On Thu, Mar 8, 2018 at 10:01 AM, Robert Dailey  wrote:
> I'm on Windows and core.ignorecase is set to 'true' when I clone/init
> a repository. I've got a branch where I started making changes to a
> file AND renamed it only to change its case. The changes I've made
> were significant enough that git no longer detects a rename, instead
> the files show up as "D" and "A" in git status (deleted then added).
> To correct this, I do an interactive rebase to add an additional
> commit before the first one to rename the file without changing it,
> and *then* allow the second commit to change the file. The goal is
> that rebase should detect the rename and automatically move the
> changes in the (now) second commit to the newly named file. Here's a
> MCVE (treat this as a script):
>
> #/bin/bash
> git init testgitrepo
> cd testgitrepo/
> git config core.ignorecase true # This is set by Windows for me, but
> hopefully will allow this to repro on linux. Didn't test linux though.
> echo "first change" > foo.txt
> git add . && git commit -m 'first change'
> git checkout -b topic
> echo "second change" > foo.txt
> git mv foo.txt FOO.txt
> git add . && git commit -m 'second change'
> git rebase -i master # Move line 1 to line 2, and put "x false" in line 1
> git mv foo.txt FOO.txt && git commit -m 'rename foo'
> git rebase --continue
> git mergetool
>
> After the rebase continue, you will get a conflict like so:
>
> error: could not apply 527d208... second change
>
> When you have resolved this problem, run "git rebase --continue".
> If you prefer to skip this patch, run "git rebase --skip" instead.
> To check out the original branch and stop rebasing, run "git rebase --abort".
>
> Could not apply 527d208... second change
> CONFLICT (rename/delete): foo.txt deleted in 527d208... second change
> and renamed to FOO.txt in HEAD. Version HEAD of FOO.txt left in tree.
>
> The last command, `git mergetool` runs, giving you the option to pick
> the Created (left) or Deleted (right) version of the file:
>
> Left: The file is created, but selecting this erases the changes from
> the "added" version on the remote (which is topic). Basically the
> rename of only case confused git, and we lost the changes on the
> remote version of the file
> Right: File is deleted. Changes are still lost.
>
> The ideal outcome is that the changes from the "added" version of the
> file in the 2nd commit get carried over to the "renamed" version of
> the file, which when you compare the two are named exactly the same
> after the 1st commit is introduced. How can I solve this issue?

I wanted to bump this and see if anyone would have a spare few minutes
to help me out with this. I know it's a needlessly complex situation,
but I'd really love to know how to resolve the specific issue, or
perhaps learn a better way to approach this in the future if I'm
shooting myself in the foot here.

Thanks again.

Will Coverity for FOSS be in scheduled maintenance mode forever?

[Johannes Schindelin]

Hi Coverity (now Synopsys) team,

I probably managed to miss important mails such as announcing that the
Coverity service for FOSS projects would be unavailable for some time.

Since February 20th, this seems to be the case (all attempts at even
looking at past results redirects to https://brb.synopsys.com/ without any
helpful further information), at least for the Git for Windows project.

Is the deal off? Or will Coverity for FOSS come back? If the latter, when?

Thanks,
Johannes

Why don't we symlink libexec/git-core/* to bin/git?

[Ævar Arnfjörð Bjarmason]

On Thu, Mar 08 2018, Daniel Jacques jotted:

>> It would be great to have this rebooted now that my perl cleanup efforts
>> have un-blocked this. Will be happy to help review & test the next
>> iteration.
>
> Yes, I was just thinking the same thing. I wanted to make sure the Perl
> changes had landed, and I'm pleased to see that they have. I should have
> time in the next few days to rebase and put up a new version of the patch
> series. I'll keep you in the loop, and thanks for pinging!

Related to this, I came across this bug report
https://gitlab.com/gitlab-org/omnibus-gitlab/issues/3265 which is
wondering why we're installing N copies of the git binary, presumably
they're building with NO_INSTALL_HARDLINKS.

Just doing this:

diff --git a/Makefile b/Makefile
index de4b8f0c02..319a4f 100644
--- a/Makefile
+++ b/Makefile
@@ -2596,7 +2596,7 @@ endif
  for p in git$X $(filter 
$(install_bindir_programs),$(ALL_PROGRAMS)); do \
$(RM) "$$execdir/$$p" && \
test -z 
"$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" && \
-   ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
+   ln -s "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
  done; \
} && \

Seems to work for me, although obviously this would need to be optional,
and it'll get in the way of Daniel's patch since it use the absolute
path.

But is there any reason anyone can think of for why we shouldn't be
figuring out the relative path and symlinking the two?

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Phillip Wood]

On 08/03/18 17:53, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
>> to mean everything from 'n' to the last line though.
> 
> Thanks for double checking.  It would be a better endgame to follow
> up with an update to existing "range selection" code to also support
> "n-", if you go that route.
> 
I'm afraid I'm not sure exactly what you're suggesting. At the moment
the range selection code is in the first patch and supports incomplete
ranges. Are you suggesting that support for incomplete ranges should be
in a separate patch or have I misunderstood?

Thanks

Phillip

Re: allow "~" to be present in a tag name

[Michal Novotny]

On Tue, Mar 13, 2018 at 11:09 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Tue, Mar 13 2018, Michal Novotny jotted:
>
>> On Tue, Mar 13, 2018 at 10:07 AM, Ævar Arnfjörð Bjarmason
>>  wrote:
>>>
>>> On Tue, Mar 13 2018, Michal Novotny jotted:
>>>
 Hello,

 currently, if I try to create a tag that has tilde "~"  in name, an
 error is raised. E.g.

 $ git tag rpkg-util-1.4~rc1
 fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.

 Now, actually it would be very cool if tilde was allowed in a tag name
 because we would like to use it for tagging pre-releases of (not-only
 rpm) packages.

 Is there some deep technical reason why tilde cannot be present in a
 tag name? I tried that e.g.
>>>
>>> Yes, because a trailing tilde is part of git's rev syntax, see "man
>>> git-rev-parse", or try in any repo:
>>>
>>> git show HEAD
>>> git show HEAD~2
>>> git show HEAD^~2
>>
>> Right, reading the man pages:
>>
>> ~, e.g. master~3
>>A suffix ~ to a revision parameter means the commit
>> object that is the th generation ancestor of the named commit
>> object, following only the first
>>parents. I.e.  ~3 is equivalent to ^^^ which is
>> equivalent to ^1^1^1. See below for an illustration of the usage
>> of this form.
>>
>> Would it be acceptable to disallow only ~ ( as [0-9]+) in a tag
>> name but allow ~[^0-9].*, i.e. if the immediately following symbol
>> after '~' is a letter, do not
>> interpret ~ as a special character. Could it work?
>
> We could make that work, with some caveats:
>
>  1) The syntax we've reserved for refnames is quite small, and my bias
> at least would be to say you should just make a tag like
> rpkg-util-1.4-rc1 instead (as e.g. git.git and linux.git do).

There is kind of clash of symbolics with rpm world. In rpm world, the component
after the last dash in a package full (versioned) name is usually
reserved for a downstream
packager who increments it as he/she adds patches to the original
sources. I will
need to do slightly more research here of what is possible.

Thank you so far!

>
> Carving out an exception like this also means we couldn't use
> ~[^0-9].* for anything magical in the future.
>
> But I think that's a rather small objection, we have other syntax
> escape hatches, and we're unlikely to use ~[^0-9].* as some new
> magic.
>
>  2) If we patch git to accept this, you'll be creating refs that aren't
> inter-operable with previous versions of git.
>
> This is a big deal. E.g. you'll happily create this special ref,
> then try to push it to github, and they'll croak because that's an
> invalid ref to them. Ditto some co-worker of yours who's using an
> older version of git.
>
> FWIW if you manually create such a tag e.g. for-each-ref will emit
> 'warning: ignoring ref with broken name' and just not show it.
>
>>>
>>> etc.
>>>
>>> Although I guess git could learn to disambiguate that form from the tag
>>> you're trying to create.
>>>
 git tag rpkg-util-1.4%rc1

 but percentage sign does not seem to be particular fitting for
 pre-release marking.

 Thank you
 clime

Re: Bug in "revision.c: --all adds HEAD from all worktrees" ?

[Duy Nguyen]

On Mon, Mar 12, 2018 at 10:39 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Now, obviously the root cause is that the repo is corrupt through some
> other bug (since fixed?), but the error message here is really bad, and
> should at least say "fatal: bad object HEAD in worktree blah" or
> something like that.

I know. I know. I'm working on it. I hit a wall and had to re-think my
approach, but I think it should be ok this time. Will be posting a
series soonish, which should also fix fsck ignoring HEADs from other
worktrees.
-- 
Duy

...

[Lindsey]

, My name is Lindsey and I'm glad to get In Touch with you after view
you profile for serious relationship,age does not matter but love and
care,

[RFC 0/4] ref-filter: remove printing from formatting logic

[Оля Тележная]

The main idea of the patch is, if you want to format the output by
ref-filter, you should have an ability to work with errors by yourself
if you want to.
So I decided not to touch signature of show_ref_array_item(), but to
move all printing (I mean errors) to it. So that we could invoke
format_ref_array_item() and be sure that we cold handle errors by
ourselves.

The patch is not finished, but I decided to show it to you. There are
still many places where we could die in the middle of formatting
process. But, if you like the general idea, I will finish and re-send
it.

Another question is about verify_ref_format(). Do we need to allow its
users also to manage errors by themselves? I left the old scenario,
printing everything in verify_ref_format() and die. If you have better
ideas, please share them.

Thanks a lot!
Olga

[RFC 4/4] ref-filter: add return value to parsers

[Olga Telezhnaya]

Continue removing any printing from ref-filter formatting logic,
so that it could be more general.

Change the signature of parsers by adding return value and
strbuf parameter for error message.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 177 +++
 1 file changed, 118 insertions(+), 59 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e146215bf1e64..06eb95e7c2c07 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -101,22 +101,28 @@ static struct used_atom {
 } *used_atom;
 static int used_atom_cnt, need_tagged, need_symref;
 
-static void color_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *color_value)
+static int color_atom_parser(const struct ref_format *format, struct used_atom 
*atom,
+const char *color_value, struct strbuf *err)
 {
-   if (!color_value)
-   die(_("expected format: %%(color:)"));
-   if (color_parse(color_value, atom->u.color) < 0)
-   die(_("unrecognized color: %%(color:%s)"), color_value);
+   if (!color_value) {
+   strbuf_addstr(err, _("expected format: %(color:)"));
+   return -1;
+   }
+   if (color_parse(color_value, atom->u.color) < 0) {
+   strbuf_addf(err, _("unrecognized color: %%(color:%s)"), 
color_value);
+   return -1;
+   }
/*
 * We check this after we've parsed the color, which lets us complain
 * about syntactically bogus color names even if they won't be used.
 */
if (!want_color(format->use_color))
color_parse("", atom->u.color);
+   return 0;
 }
 
-static void refname_atom_parser_internal(struct refname_atom *atom,
-const char *arg, const char *name)
+static int refname_atom_parser_internal(struct refname_atom *atom, const char 
*arg,
+const char *name, struct strbuf *err)
 {
if (!arg)
atom->option = R_NORMAL;
@@ -125,17 +131,25 @@ static void refname_atom_parser_internal(struct 
refname_atom *atom,
else if (skip_prefix(arg, "lstrip=", ) ||
 skip_prefix(arg, "strip=", )) {
atom->option = R_LSTRIP;
-   if (strtol_i(arg, 10, >lstrip))
-   die(_("Integer value expected refname:lstrip=%s"), arg);
+   if (strtol_i(arg, 10, >lstrip)) {
+   strbuf_addf(err, _("Integer value expected 
refname:lstrip=%s"), arg);
+   return -1;
+   }
} else if (skip_prefix(arg, "rstrip=", )) {
atom->option = R_RSTRIP;
-   if (strtol_i(arg, 10, >rstrip))
-   die(_("Integer value expected refname:rstrip=%s"), arg);
-   } else
-   die(_("unrecognized %%(%s) argument: %s"), name, arg);
+   if (strtol_i(arg, 10, >rstrip)) {
+   strbuf_addf(err, _("Integer value expected 
refname:rstrip=%s"), arg);
+   return -1;
+   }
+   } else {
+   strbuf_addf(err, _("unrecognized %%(%s) argument: %s"), name, 
arg);
+   return -1;
+   }
+   return 0;
 }
 
-static void remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom, const char *arg)
+static int remote_ref_atom_parser(const struct ref_format *format, struct 
used_atom *atom,
+ const char *arg, struct strbuf *err)
 {
struct string_list params = STRING_LIST_INIT_DUP;
int i;
@@ -145,9 +159,8 @@ static void remote_ref_atom_parser(const struct ref_format 
*format, struct used_
 
if (!arg) {
atom->u.remote_ref.option = RR_REF;
-   refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
-   return;
+   return refname_atom_parser_internal(>u.remote_ref.refname,
+   arg, atom->name, err);
}
 
atom->u.remote_ref.nobracket = 0;
@@ -170,29 +183,40 @@ static void remote_ref_atom_parser(const struct 
ref_format *format, struct used_
atom->u.remote_ref.push_remote = 1;
} else {
atom->u.remote_ref.option = RR_REF;
-   
refname_atom_parser_internal(>u.remote_ref.refname,
-arg, atom->name);
+   if 
(refname_atom_parser_internal(>u.remote_ref.refname,
+arg, atom->name, err))
+   return -1;
}
}
 
string_list_clear(, 0);
+   return 0;
 }
 
-static void body_atom_parser(const struct ref_format *format, struct used_atom 
*atom, const char 

[RFC 3/4] ref-filter: change parsing function error handling

[Olga Telezhnaya]

Continue removing any printing from ref-filter formatting logic,
so that it could be more general.

Change the signature of parse_ref_filter_atom() by changing return value,
adding previous return value to function parameter and also adding
strbuf parameter for error message.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 45 -
 1 file changed, 32 insertions(+), 13 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 07bedc636398c..e146215bf1e64 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -397,7 +397,8 @@ struct atom_value {
  * Used to parse format string and sort specifiers
  */
 static int parse_ref_filter_atom(const struct ref_format *format,
-const char *atom, const char *ep)
+const char *atom, const char *ep, int *res,
+struct strbuf *err)
 {
const char *sp;
const char *arg;
@@ -406,14 +407,18 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
sp = atom;
if (*sp == '*' && sp < ep)
sp++; /* deref */
-   if (ep <= sp)
-   die(_("malformed field name: %.*s"), (int)(ep-atom), atom);
+   if (ep <= sp) {
+   strbuf_addf(err, _("malformed field name: %.*s"), 
(int)(ep-atom), atom);
+   return -1;
+   }
 
/* Do we have the atom already used elsewhere? */
for (i = 0; i < used_atom_cnt; i++) {
int len = strlen(used_atom[i].name);
-   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len))
-   return i;
+   if (len == ep - atom && !memcmp(used_atom[i].name, atom, len)) {
+   *res = i;
+   return 0;
+   }
}
 
/*
@@ -432,8 +437,10 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
break;
}
 
-   if (ARRAY_SIZE(valid_atom) <= i)
-   die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
+   if (ARRAY_SIZE(valid_atom) <= i) {
+   strbuf_addf(err, _("unknown field name: %.*s"), (int)(ep-atom), 
atom);
+   return -1;
+   }
 
/* Add it in, including the deref prefix */
at = used_atom_cnt;
@@ -458,7 +465,8 @@ static int parse_ref_filter_atom(const struct ref_format 
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
-   return at;
+   *res = at;
+   return 0;
 }
 
 static void quote_formatting(struct strbuf *s, const char *str, int 
quote_style)
@@ -725,17 +733,20 @@ int verify_ref_format(struct ref_format *format)
 
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+   struct strbuf err = STRBUF_INIT;
const char *color, *ep = strchr(sp, ')');
int at;
 
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
-   at = parse_ref_filter_atom(format, sp + 2, ep);
+   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
+   die("%s", err.buf);
cp = ep + 1;
 
if (skip_prefix(used_atom[at].name, "color:", ))
format->need_color_reset_at_eol = !!strcmp(color, 
"reset");
+   strbuf_release();
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
@@ -2154,15 +2165,18 @@ int format_ref_array_item(struct ref_array_item *info,
 
for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
+   struct strbuf err = STRBUF_INIT;
+   int pos;
 
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, );
-   get_ref_atom_value(info,
-  parse_ref_filter_atom(format, sp + 2, ep),
-  );
+   if (parse_ref_filter_atom(format, sp + 2, ep, , ))
+   return -1;
+   get_ref_atom_value(info, pos, );
if (atomv->handler(atomv, , error_buf))
return -1;
+   strbuf_release();
}
if (*cp) {
sp = cp + strlen(cp);
@@ -2215,7 +2229,12 @@ static int parse_sorting_atom(const char *atom)
 */
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
-   return parse_ref_filter_atom(, atom, end);
+   struct strbuf err = STRBUF_INIT;
+   int res;
+   if (parse_ref_filter_atom(, atom, end, , ))
+   

[RFC 1/4] ref-filter: start adding strbufs with errors

[Olga Telezhnaya]

This is a first step in removing any printing from
ref-filter formatting logic, so that it could be more general.
Everything would be the same for show_ref_array_item() users.
But, if you want to deal with errors by your own, you could invoke
format_ref_array_item(). It means that you need to print everything
(the result and errors) on your side.

This commit changes signature of format_ref_array_item() by adding
return value and strbuf parameter for errors, and fixes
its callers.

Signed-off-by: Olga Telezhnaia 
---
 builtin/branch.c |  7 +--
 ref-filter.c | 17 -
 ref-filter.h |  7 ---
 3 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 8dcc2ed058be6..f86709ca42d5e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -391,7 +391,6 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
-   struct strbuf out = STRBUF_INIT;
char *to_free = NULL;
 
/*
@@ -419,7 +418,10 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
ref_array_sort(sorting, );
 
for (i = 0; i < array.nr; i++) {
-   format_ref_array_item(array.items[i], format, );
+   struct strbuf out = STRBUF_INIT;
+   struct strbuf err = STRBUF_INIT;
+   if (format_ref_array_item(array.items[i], format, , ))
+   die("%s", err.buf);
if (column_active(colopts)) {
assert(!filter->verbose && "--column and --verbose are 
incompatible");
 /* format to a string_list to let print_columns() do 
its job */
@@ -428,6 +430,7 @@ static void print_ref_list(struct ref_filter *filter, 
struct ref_sorting *sortin
fwrite(out.buf, 1, out.len, stdout);
putchar('\n');
}
+   strbuf_release();
strbuf_release();
}
 
diff --git a/ref-filter.c b/ref-filter.c
index 45fc56216aaa8..54fae00bdd410 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -2118,9 +2118,10 @@ static void append_literal(const char *cp, const char 
*ep, struct ref_formatting
}
 }
 
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
   const struct ref_format *format,
-  struct strbuf *final_buf)
+  struct strbuf *final_buf,
+  struct strbuf *error_buf)
 {
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
@@ -2148,19 +2149,25 @@ void format_ref_array_item(struct ref_array_item *info,
resetv.s = GIT_COLOR_RESET;
append_atom(, );
}
-   if (state.stack->prev)
-   die(_("format: %%(end) atom missing"));
+   if (state.stack->prev) {
+   strbuf_addstr(error_buf, _("format: %(end) atom missing"));
+   return -1;
+   }
strbuf_addbuf(final_buf, >output);
pop_stack_element();
+   return 0;
 }
 
 void show_ref_array_item(struct ref_array_item *info,
 const struct ref_format *format)
 {
struct strbuf final_buf = STRBUF_INIT;
+   struct strbuf error_buf = STRBUF_INIT;
 
-   format_ref_array_item(info, format, _buf);
+   if (format_ref_array_item(info, format, _buf, _buf))
+   die("%s", error_buf.buf);
fwrite(final_buf.buf, 1, final_buf.len, stdout);
+   strbuf_release(_buf);
strbuf_release(_buf);
putchar('\n');
 }
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..e13f8e6f8721a 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -110,9 +110,10 @@ int verify_ref_format(struct ref_format *format);
 /*  Sort the given ref_array as per the ref_sorting provided */
 void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
 /*  Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
-  const struct ref_format *format,
-  struct strbuf *final_buf);
+int format_ref_array_item(struct ref_array_item *info,
+ const struct ref_format *format,
+ struct strbuf *final_buf,
+ struct strbuf *error_buf);
 /*  Print the ref using the given format and quote_style */
 void show_ref_array_item(struct ref_array_item *info, const struct ref_format 
*format);
 /*  Parse a single sort specifier and add it to the list */

--
https://github.com/git/git/pull/466

[RFC 2/4] ref-filter: add return value && strbuf to handlers

[Olga Telezhnaya]

Continue removing any printing from ref-filter formatting logic,
so that it could be more general.

Change the signature of handlers by adding return value
and strbuf parameter for errors.

Signed-off-by: Olga Telezhnaia 
---
 ref-filter.c | 71 
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 54fae00bdd410..07bedc636398c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -387,7 +387,8 @@ struct ref_formatting_state {
 
 struct atom_value {
const char *s;
-   void (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state);
+   int (*handler)(struct atom_value *atomv, struct ref_formatting_state 
*state,
+  struct strbuf *err);
uintmax_t value; /* used for sorting when not FIELD_STR */
struct used_atom *atom;
 };
@@ -481,7 +482,8 @@ static void quote_formatting(struct strbuf *s, const char 
*str, int quote_style)
}
 }
 
-static void append_atom(struct atom_value *v, struct ref_formatting_state 
*state)
+static int append_atom(struct atom_value *v, struct ref_formatting_state 
*state,
+  struct strbuf *err)
 {
/*
 * Quote formatting is only done when the stack has a single
@@ -493,6 +495,7 @@ static void append_atom(struct atom_value *v, struct 
ref_formatting_state *state
quote_formatting(>stack->output, v->s, 
state->quote_style);
else
strbuf_addstr(>stack->output, v->s);
+   return 0;
 }
 
 static void push_stack_element(struct ref_formatting_stack **stack)
@@ -527,7 +530,8 @@ static void end_align_handler(struct ref_formatting_stack 
**stack)
strbuf_release();
 }
 
-static void align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int align_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+ struct strbuf *err)
 {
struct ref_formatting_stack *new_stack;
 
@@ -535,6 +539,7 @@ static void align_atom_handler(struct atom_value *atomv, 
struct ref_formatting_s
new_stack = state->stack;
new_stack->at_end = end_align_handler;
new_stack->at_end_data = >atom->u.align;
+   return 0;
 }
 
 static void if_then_else_handler(struct ref_formatting_stack **stack)
@@ -572,7 +577,8 @@ static void if_then_else_handler(struct 
ref_formatting_stack **stack)
free(if_then_else);
 }
 
-static void if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int if_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+  struct strbuf *err)
 {
struct ref_formatting_stack *new_stack;
struct if_then_else *if_then_else = xcalloc(sizeof(struct 
if_then_else), 1);
@@ -584,6 +590,7 @@ static void if_atom_handler(struct atom_value *atomv, 
struct ref_formatting_stat
new_stack = state->stack;
new_stack->at_end = if_then_else_handler;
new_stack->at_end_data = if_then_else;
+   return 0;
 }
 
 static int is_empty(const char *s)
@@ -596,19 +603,24 @@ static int is_empty(const char *s)
return 1;
 }
 
-static void then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state)
+static int then_atom_handler(struct atom_value *atomv, struct 
ref_formatting_state *state,
+struct strbuf *err)
 {
struct ref_formatting_stack *cur = state->stack;
struct if_then_else *if_then_else = NULL;
 
if (cur->at_end == if_then_else_handler)
if_then_else = (struct if_then_else *)cur->at_end_data;
-   if (!if_then_else)
-   die(_("format: %%(then) atom used without an %%(if) atom"));
-   if (if_then_else->then_atom_seen)
-   die(_("format: %%(then) atom used more than once"));
-   if (if_then_else->else_atom_seen)
-   die(_("format: %%(then) atom used after %%(else)"));
+   if (!if_then_else) {
+   strbuf_addstr(err, _("format: %(then) atom used without an 
%(if) atom"));
+   return -1;
+   } else if (if_then_else->then_atom_seen) {
+   strbuf_addstr(err, _("format: %(then) atom used more than 
once"));
+   return -1;
+   } else if (if_then_else->else_atom_seen) {
+   strbuf_addstr(err, _("format: %(then) atom used after 
%(else)"));
+   return -1;
+   }
if_then_else->then_atom_seen = 1;
/*
 * If the 'equals' or 'notequals' attribute is used then
@@ -624,34 +636,44 @@ static void then_atom_handler(struct atom_value *atomv, 
struct ref_formatting_st
} else if (cur->output.len && !is_empty(cur->output.buf))
if_then_else->condition_satisfied = 1;
strbuf_reset(>output);
+   return 0;
 }
 
-static void else_atom_handler(struct atom_value *atomv, 

Re: allow "~" to be present in a tag name

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Michal Novotny jotted:

> On Tue, Mar 13, 2018 at 10:07 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>>
>> On Tue, Mar 13 2018, Michal Novotny jotted:
>>
>>> Hello,
>>>
>>> currently, if I try to create a tag that has tilde "~"  in name, an
>>> error is raised. E.g.
>>>
>>> $ git tag rpkg-util-1.4~rc1
>>> fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.
>>>
>>> Now, actually it would be very cool if tilde was allowed in a tag name
>>> because we would like to use it for tagging pre-releases of (not-only
>>> rpm) packages.
>>>
>>> Is there some deep technical reason why tilde cannot be present in a
>>> tag name? I tried that e.g.
>>
>> Yes, because a trailing tilde is part of git's rev syntax, see "man
>> git-rev-parse", or try in any repo:
>>
>> git show HEAD
>> git show HEAD~2
>> git show HEAD^~2
>
> Right, reading the man pages:
>
> ~, e.g. master~3
>A suffix ~ to a revision parameter means the commit
> object that is the th generation ancestor of the named commit
> object, following only the first
>parents. I.e.  ~3 is equivalent to ^^^ which is
> equivalent to ^1^1^1. See below for an illustration of the usage
> of this form.
>
> Would it be acceptable to disallow only ~ ( as [0-9]+) in a tag
> name but allow ~[^0-9].*, i.e. if the immediately following symbol
> after '~' is a letter, do not
> interpret ~ as a special character. Could it work?

We could make that work, with some caveats:

 1) The syntax we've reserved for refnames is quite small, and my bias
at least would be to say you should just make a tag like
rpkg-util-1.4-rc1 instead (as e.g. git.git and linux.git do).

Carving out an exception like this also means we couldn't use
~[^0-9].* for anything magical in the future.

But I think that's a rather small objection, we have other syntax
escape hatches, and we're unlikely to use ~[^0-9].* as some new
magic.

 2) If we patch git to accept this, you'll be creating refs that aren't
inter-operable with previous versions of git.

This is a big deal. E.g. you'll happily create this special ref,
then try to push it to github, and they'll croak because that's an
invalid ref to them. Ditto some co-worker of yours who's using an
older version of git.

FWIW if you manually create such a tag e.g. for-each-ref will emit
'warning: ignoring ref with broken name' and just not show it.

>>
>> etc.
>>
>> Although I guess git could learn to disambiguate that form from the tag
>> you're trying to create.
>>
>>> git tag rpkg-util-1.4%rc1
>>>
>>> but percentage sign does not seem to be particular fitting for
>>> pre-release marking.
>>>
>>> Thank you
>>> clime

Re: Opinions on changing add/add conflict resolution?

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Elijah Newren jotted:

> Hi,
>
> On Mon, Mar 12, 2018 at 3:19 PM, Ævar Arnfjörð Bjarmason 
> wrote:
>
>>
>> Does this mean that e.g. in this case of merging two files, one
>> containing "foo" and one containing "bar":
>>
>> (
>> rm -rf /tmp/test.git &&
>> git init /tmp/test.git &&
>> cd /tmp/test.git &&
>> echo foo >README &&
>> git add README &&
>> git commit -mfoo &&
>> git checkout --orphan trunk &&
>> git reset --hard &&
>> echo bar >README &&
>> git add README &&
>> git commit -mbar &&
>> git merge --allow-unrelated-histories master;
>> cat README
>> )
>>
>> That instead of getting:
>>
>> <<< HEAD
>> bar
>> ===
>> foo
>> >>> master
>>
>> I'd now get these split into different files?
>>
>
> As currently implemented, yes.  However, I was more concerned the idea of
> handling files differently based on whether or not they were similar,
> rather than on what the precise definition of "similar" is for this context.
>
> As far as the definition of similarity goes, estimate_similarity() is
> currently used by rename detection to compare files recorded at different
> pathnames.  By contrast, in this context, we are comparing two files which
> were recorded with the same pathname.  That suggests the heuristic could be
> a bit different and use more than just estimate_similarity().  (e.g. "We
> consider these files similar IF >50% of the lines match OR both files are
> less than 2K.")

Yeah that's fair enough, we could definitely tweak a dedicated heuristic
for just this purpose to do the right thing most of the time, although
really having completely unrelated 10-line README files use the old
two-way merge would, I guess, also be confusing to users who want this
feature.

>> I don't mind this being a configurable option if you want it, but I
>> don't think it should be on by default, reasons:
>>
>
> I don't think a configurable option makes sense, at least not for my
> purposes.  Having rename/rename conflicts be "safely" mis-detected as
> rename/add or add/add, and having rename/add conflicts be "safely"
> mis-detected as add/add is my overriding concern.  Thus, making these three
> conflict types behave consistently is what I need.  Options would make that
> more difficult for me, and would thus feel like a step backwards.

I think there's three types of users who interact with the current
format:

 1) The user using no special tool (e.g. editing with nano) and runs
into a conflict, maybe this helps these users the most.

 2) The user who's also using no special tool (e.g. editing with nano)
but just prefers to get in-file conflict markers.

 3) The user using a tool to solve the conflict, who doesn't care if
we'd spew out our current format, this new formar, or some XML file
with both versions or whatevery, they're using some other UI.

I don't think we should make some change that breaks existing tools for
users of #3 by default.

>  1) There's lots of cases where we totally screw up the "is this
>> similar?" check, in particular with small files.
>>
>> E.g. let's say you have a config file like 'fs-path "/tmp/git"' and
>> in two branches you change that to 'fs-path "/opt/git"' and 'fs-path
>> "/var/git"'. The rename detection will think this these have nothing
>> to do with each other since they share no common lines, but to a
>> human reader they're really similar, and would make sense in the
>> context of resolving a bigger merge where /{opt,var}/git changes are
>> conflicting.
>>
>> This is not some theoretical concern, there's lots of things that
>> e.g. use small 5-10 line config files to configure some app that
>> because of some combo of indentation changes and changing a couple
>> of lines will make git's rename detection totally give up, but to a
>> human reader they're 95% the same.
>>
>
> Fair enough.  The small files case could potentially be handled by just
> changing the similarity metric for these conflict types, as noted above.
> If it's a small file, that might be the easiest way for a user to deal with
> it too.

Or it might be easier for the user to deal with even big conflicts with
in-file conflict markers, see use-case #2 & #3 above.

> I'm not sure I see the problem with the bigger files, though.  If you have
> bigger files with less than 50% of the lines matching, then you'll
> essentially end up with a great big conflict block with one file on one
> side and the other file on the other side, which doesn't seem that
> different to me than having them be in two separate files.  In fact,
> separate files seems easier to deal with because then the user can run e.g.
> 'git diff --no-index --color-words FILE1 FILE2', something that they can't
> do when it's in one file.  That has bothered me more than once, and made me
> wish they were 

Re: allow "~" to be present in a tag name

[Michal Novotny]

On Tue, Mar 13, 2018 at 10:07 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Tue, Mar 13 2018, Michal Novotny jotted:
>
>> Hello,
>>
>> currently, if I try to create a tag that has tilde "~"  in name, an
>> error is raised. E.g.
>>
>> $ git tag rpkg-util-1.4~rc1
>> fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.
>>
>> Now, actually it would be very cool if tilde was allowed in a tag name
>> because we would like to use it for tagging pre-releases of (not-only
>> rpm) packages.
>>
>> Is there some deep technical reason why tilde cannot be present in a
>> tag name? I tried that e.g.
>
> Yes, because a trailing tilde is part of git's rev syntax, see "man
> git-rev-parse", or try in any repo:
>
> git show HEAD
> git show HEAD~2
> git show HEAD^~2

Right, reading the man pages:

~, e.g. master~3
   A suffix ~ to a revision parameter means the commit
object that is the th generation ancestor of the named commit
object, following only the first
   parents. I.e.  ~3 is equivalent to ^^^ which is
equivalent to ^1^1^1. See below for an illustration of the usage
of this form.

Would it be acceptable to disallow only ~ ( as [0-9]+) in a tag
name but allow ~[^0-9].*, i.e. if the immediately following symbol
after '~' is a letter, do not
interpret ~ as a special character. Could it work?

Thank you!
clime

>
> etc.
>
> Although I guess git could learn to disambiguate that form from the tag
> you're trying to create.
>
>> git tag rpkg-util-1.4%rc1
>>
>> but percentage sign does not seem to be particular fitting for
>> pre-release marking.
>>
>> Thank you
>> clime

Re: allow "~" to be present in a tag name

[Ævar Arnfjörð Bjarmason]

On Tue, Mar 13 2018, Michal Novotny jotted:

> Hello,
>
> currently, if I try to create a tag that has tilde "~"  in name, an
> error is raised. E.g.
>
> $ git tag rpkg-util-1.4~rc1
> fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.
>
> Now, actually it would be very cool if tilde was allowed in a tag name
> because we would like to use it for tagging pre-releases of (not-only
> rpm) packages.
>
> Is there some deep technical reason why tilde cannot be present in a
> tag name? I tried that e.g.

Yes, because a trailing tilde is part of git's rev syntax, see "man
git-rev-parse", or try in any repo:

git show HEAD
git show HEAD~2
git show HEAD^~2

etc.

Although I guess git could learn to disambiguate that form from the tag
you're trying to create.

> git tag rpkg-util-1.4%rc1
>
> but percentage sign does not seem to be particular fitting for
> pre-release marking.
>
> Thank you
> clime

allow "~" to be present in a tag name

[Michal Novotny]

Hello,

currently, if I try to create a tag that has tilde "~"  in name, an
error is raised. E.g.

$ git tag rpkg-util-1.4~rc1
fatal: 'rpkg-util-1.4~rc1' is not a valid tag name.

Now, actually it would be very cool if tilde was allowed in a tag name
because we would like to use it for tagging pre-releases of (not-only
rpm) packages.

Is there some deep technical reason why tilde cannot be present in a
tag name? I tried that e.g.

git tag rpkg-util-1.4%rc1

but percentage sign does not seem to be particular fitting for
pre-release marking.

Thank you
clime

Re: [RFC] Rebasing merges: a jorney to the ultimate solution(RoadClear)

[Sergey Organov]

Hi Buga,

Igor Djordjevic  writes:

[...]

> I don`t know, I`m thinking if we are looking at todo list from
> different perspectives - to you, it seems to be a sequence of
> commands to create something new (yes, from something that already
> exists, but that`s implementation detail). In that context, using
> `merge` might have sense (even if still being a very special merge).
>
> But to me, as we already have `pick` and not `commit` to rebase
> commits, it means we are not creating but rather reusing what we have
> (being an important concept to reason about), thus `pick` would still
> fit in for picking a merge commit just nicely, and more naturally,
> too.

Exactly.

Fundamentally, a good editing tool should first be able to just safely
reproduce back what it got, only then everything else goes.

>From this follows that git history-editing tool should start from sound
history representation, i.e., some text representation of the DAG that
allows to re-create the DAG back.

The simplest way is to just list all the nodes in some topological
order, along with references to the parents. Then, to simplify the list,
first parent, unless explicitly specified, could be assumed to be the
preceding item in the list.

Next, we actually need _to do_ something with this, so we convert this
to a _todo_ list by prepending action to each element of the list (isn't
it Lisp once again?). Let the action be called 'pick'. Then, e.g., the
piece of history:

   B1---B2
  /   \
 S--M0---M1 M2 M3 -- M4

from M0 to M4 will be represented like this:

skip S  # Just to have a handy reference
pick M0 # Implicit first parent (S)
pick M1 # Implicit first parent (M0)
pick M2 # Implicit first parent (M1)
pick B1 M1  # Explicit first parent
pick B2 # Implicit first parent (B1)
pick M3 M2 B2   # Explicit first and second parents
pick M4

Which basically gets us back to what you are advocating.

Here is another variant, using command options to specify parents (I
also exchanged order of branch and mainline):

skip S  # To have the base reference handy
pick M0 # Implicit first parent (S)
pick B1 # Implicit first parent (M0)
pick B2 # Implicit first parent (B1)
pick M1 -1 M0   # Explicit first parent M0
pick M2 # Implicit first parent (M1)
pick M3 -2 B2   # Implicit first parent (M2) and
# explicit second parent B2
pick M4 # Implicit first parent (M3)

I like this one even better.

IMHO, this is indeed a good starting point. No special treatment for
merges is needed so far.

-- Sergey