Re: [PATCH 3/3] branch: forbid refs/heads/HEAD

[Kaartic Sivaraam]

On Sat, 2017-10-21 at 17:57 +0900, Junio C Hamano wrote:
> ... The code may already
> handle it, or there may need even more code to support the rename; I
> didn't check.
> 

As far as I could see there the code does seem to already handle the
rename. This part of builtin/branch.c is what seems to be ensuring
that,

if (strbuf_check_branch_ref(, oldname)) {
/*
 * Bad name --- this could be an attempt to rename a
 * ref that we used to allow to be created by accident.
 */
if (ref_exists(oldref.buf))
recovery = 1;
else
die(_("Invalid branch name: '%s'"), oldname);
}


-- 
Kaartic

Re: [PATCH 0/3] a small branch API clean-up

[Kaartic Sivaraam]

On Sat, 2017-10-21 at 17:52 +0900, Junio C Hamano wrote:
> 
> Sorry, but I am not sure what you are asking.  
> 
> I was looking at the code, noticed NEEDSWORK comment and worked on
> cleaning things up---you seem to feel that I need a reason, or
> perhaps even your permission, when I try improving the codebase?
> That starts to sound a bit ridiculous.

Nothing like that, sorry. I was thinking that you would "remember" the
fact that I was trying to clean up the NEEDSWORK in the patch series
mentioned before as you have reviewed/commented on it. So, I thought
there would be something my patch series was doing wrong which made you
send another series that cleans up the NEEDSWORK. I just wanted to know
that specific reason (the reason which made you send a series cleaning
up the NEEDSWORK when you saw another series doing the same thing)?

Of course that's assuming that you remembered my series cleaning up the
NEEDSWORK, in the first place. If that's not the case then there's no
reason you could give :-)

-- 
Kaartic

Re: [PATCH v3 4/4] status: test --ignored=matching

[Junio C Hamano]

Jameson Miller  writes:

> Add tests around status reporting ignord files that match an exclude
> pattern for both --untracked-files=normal and --untracked-files=all.
>
> Signed-off-by: Jameson Miller 
> ---
>  t/t7519-ignored-mode.sh | 183 
> 
>  1 file changed, 183 insertions(+)
>  create mode 100755 t/t7519-ignored-mode.sh

Ben's fsmonitor series already uses t7519, so please move this
somewhere else.  Perhaps

git diff --name-only --diff-filter=A master pu t/

would help you see what new tests that are missing from 'master'
are added by topics in flight.

Re: [PATCH 0/6] Create Git/Packet.pm

[Junio C Hamano]

Christian Couder  writes:

> Goal
> 

Totally offtopic, but is it only me who finds these "section
headers" in cover letters from some people irritating and/or
jarring?  It perhaps is because I view the cover letter more as a
part of conversation, not a presentation.  And when you walk up to
somebody and start a conversation, you do not declare section
headers ;-)  

Saying "I want to be able to do these things in the future, and here
is to prepare for that future" at the beginning nevertheless is a
good thing.  It gives us readers an overall vision we can agree to
(or be against, or offer alternatives) and sets expectations on what
the series would do and where it stops and leaves the remainder to
follow-up work.

> Packet related functions in Perl can be useful to write new filters or
> to debug or test existing filters. So instead of having them in
> t0021/rot13-filter.pl, let's extract them into a new Git/Packet.pm
> module.

I left some comments on individual patches to point out places that
may need improvements.  I agree with the overall direction.

Thanks for starting this topic.

Re: [PATCH 5/6] t0021/rot13-filter: add capability functions

[Junio C Hamano]

Christian Couder  writes:

> Add functions to help read and write capabilities.
> These functions will be reused in following patches.

One more thing that is more noteworthy (read: do not forget to
describe it in the proposed log message) is that the original used
to require capabilities to come in a fixed order.

The new code allows these capabilities to be declared in any order,
it even allows duplicates (intended? shouldn't we be flagging it as
an error?), the helper can require a set of capabilities we do want
to see and fail if the remote doesn't declare any one of them
(good).

It does not check if the remote declares any capability we do not
know about (intended? the original noticed this situation and error
out---shouldn't the more generalized helper that is meant to be
reusable allow us to do so, too?).

Side note: my answer to the last question is "it is OK and
even desirable to ignore the fact that they claim to support
a capability we do not know about", but I may be mistaken.
The reasoning and the conclusion that is consistent with
what the code does should be described in any case.

> +sub packet_read_capabilities {
> + my @cap;
> + while (1) {
> + my ( $res, $buf ) = packet_bin_read();
> + return ( $res, @cap ) if ( $res != 0 );

The original had the same "'list eq list' does not do what you may
think it does" issue.  This one corrects it, which is good.

I am not sure if ($res != 0) is correct though.  What should happen
when you get an unexpected EOF at this point?  The original would
have died; this ignores and continues.

> + unless ( $buf =~ s/\n$// ) {
> + die "A non-binary line MUST be terminated by an LF.\n"
> + . "Received: '$buf'";
> + }

It may make sense to extract this in a small helper and call it from
here and from packet_txt_read().

> + die "bad capability buf: '$buf'" unless ( $buf =~ 
> s/capability=// );

This may merely be a style thing, but I somehow find statement
modifiers hard to follow, unless its condition is almost always
true.  If you follow the logic in a loop and see "die" at the
beginning, a normal thing to expect is that there were conditionals
that said "continue" (eh, 'next' or 'redo') to catch the normal case
and the control would reach "die" only under exceptional error
cases, but hiding a rare error condition after 'unless' statement
modifier breaks that expectation.

> + push @cap, $buf;
> + }
> +}
> +
> +sub packet_read_and_check_capabilities {
> + my @local_caps = @_;
> + my @remote_res_caps = packet_read_capabilities();
> + my $res = shift @remote_res_caps;
> + my %remote_caps = map { $_ => 1 } @remote_res_caps;

FYI:

my ($res, @remote_caps) = packet_read_capabilities();
my %remote_caps = map { $_ => 1 } @remote_caps;

may be more conventional way.

> + foreach (@local_caps) {
> + die "'$_' capability not available" unless 
> (exists($remote_caps{$_}));
> + }

It is good that we can now accept capabilities in any order and
still enforce all the required capabilities are supported by the
other side.  It deserves a mention in the proposed log message.

> + return $res;
> +}
> +
> +sub packet_write_capabilities {
> + packet_txt_write( "capability=" . $_ ) foreach (@_);
> + packet_flush();
> +}
> +
>  print $debug "START\n";
>  $debug->flush();
>  
>  packet_initialize("git-filter", 2);
>  
> -( packet_txt_read() eq ( 0, "capability=clean" ) )  || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=smudge" ) ) || die "bad capability";
> -( packet_txt_read() eq ( 0, "capability=delay" ) )  || die "bad capability";
> -( packet_bin_read() eq ( 1, "" ) )  || die "bad capability 
> end";
> +packet_read_and_check_capabilities("clean", "smudge", "delay");
> +packet_write_capabilities(@capabilities);

Neither the original nor the rewrite ensures that @capabilities we
ask to the other side to activate is a subset of what the other side
actually declared.

Fixing this is a bit more involved than "refactor what we have", but
probably is part of "refactor so that we can reuse in other
situations".  You'd want to return the list of caps received from
packet_read_and_check_capabilities() and make sure @capabilities is
a subset of that before writing them out to the other side to
request.

Thanks.

Re: [PATCH 4/6] t0021/rot13-filter: add packet_initialize()

[Junio C Hamano]

Christian Couder  writes:

> +sub packet_initialize {
> + my ($name, $version) = @_;
> +
> + ( packet_txt_read() eq ( 0, $name . "-client" ) )   || die "bad 
> initialize";
> + ( packet_txt_read() eq ( 0, "version=" . $version ) )   || die "bad 
> version";
> + ( packet_bin_read() eq ( 1, "" ) )  || die "bad 
> version end";

This is not a new issue and it is a bit surprising that nobody
noticed when edcc8581 ("convert: add filter..process
option", 2016-10-16) was reviewed, but the above is quite broken.
packet_txt_read() returns a 2-element list, and on the right hand
side of "eq" also has a list with (two, elements), but this takes
the last element of the list on each side, and compares them.  The
elading 0/1 we see above do not matter, which means we do not require
to see a flush at the end of the version---a simple empty string or
an EOF would do, which is definitely not what we want.

#!/usr/bin/perl
sub p {
my ($res, $buf) = @_;
return ($res, $buf);
}
if (p(0, "") eq (-1, 2, 3, "")) { print "ok\n"; }

It is fine to leave the original code broken at this step while we
purely move the lines around, and hopefully this will be corrected
in a later step in the series (I am responding as I read on, so I do
not yet know at which patch that happens in this series).

Thanks.

Re: [PATCH 1/6] t0021/rot13-filter: refactor packet reading functions

[Junio C Hamano]

Christian Couder  writes:

> To make it possible in a following commit to move packet
> reading and writing functions into a Packet.pm module,
> let's refactor these functions, so they don't handle
> printing debug output and exiting.
>
> Signed-off-by: Christian Couder 
> ---
>  t/t0021/rot13-filter.pl | 12 
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
> index ad685d92f8..e4495a52f3 100644
> --- a/t/t0021/rot13-filter.pl
> +++ b/t/t0021/rot13-filter.pl
> @@ -60,8 +60,7 @@ sub packet_bin_read {
>   my $bytes_read = read STDIN, $buffer, 4;
>   if ( $bytes_read == 0 ) {
>   # EOF - Git stopped talking to us!
> - print $debug "STOP\n";
> - exit();
> + return ( -1, "" );
>   }
>   elsif ( $bytes_read != 4 ) {
>   die "invalid packet: '$buffer'";
> @@ -85,7 +84,7 @@ sub packet_bin_read {
>  
>  sub packet_txt_read {
>   my ( $res, $buf ) = packet_bin_read();
> - unless ( $buf eq '' or $buf =~ s/\n$// ) {
> + unless ( $res == -1 or $buf eq '' or $buf =~ s/\n$// ) {
>   die "A non-binary line MUST be terminated by an LF.";
>   }
>   return ( $res, $buf );
> @@ -131,7 +130,12 @@ print $debug "init handshake complete\n";
>  $debug->flush();
>  
>  while (1) {
> - my ( $command ) = packet_txt_read() =~ /^command=(.+)$/;
> + my ( $res, $command ) = packet_txt_read();
> + if ( $res == -1 ) {
> + print $debug "STOP\n";
> + exit();
> + }
> + $command =~ s/^command=//;
>   print $debug "IN: $command";
>   $debug->flush();

This was not an issue in the old code which died upon unexpected EOF
inside the lowest-level helper packet_bin_read(), but now you have
one call to packet_bin_read() and many calls to packet_txt_read()
whose return value is not checked for this new condition you are
allowing packet_bin_read() to return.  This step taken alone is a
regression---let's see how the remainder of the series updates the
callers to compensate.

I initially thought that it may be more Perl-ish to return undef or
string instead of returning a 2-element list, but this code needs to
distinguish three conditions (i.e. a normal string that is 0 or more
bytes long, a flush, and an EOF), so that is not sufficient.  Perl
experts on the list still may be able to suggest a better way than
the current one to do so, but that is outside the scope of this
refactoring.

Thanks for starting to work on this.

Re: [PATCH v4] commit: check result of resolve_ref_unsafe

[Junio C Hamano]

Jeff King  writes:

>> Hello,
>> I think this way is better for user experience:
>> * git doesn't crash;
>> * warning is shown;
>> * commit has been successfully created then it's safe to show a summary 
>> message
>> with already known information and without resolved HEAD.
>
> I'm on the fence between this and the die_errno() version. Given that
> this would basically never happen in practice, I don't think it matters
> too much. And that makes me want to just err on the side of simplicity.
> But it's not like this is all that complex, either.

True.  I've queued v3 for now.

Thanks, both.

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Junio C Hamano]

Jeff King  writes:

> On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:
>
>> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King  wrote:
>> > The refs_resolve_ref_unsafe() function may return NULL even
>> > with a REF_ISSYMREF flag if a symref points to a broken ref.
>> > As a result, it's possible for find_shared_symref() to
>> > segfault when it passes NULL to strcmp().
>> > [...]
>> > We can fix this by treating NULL the same as a non-matching
>> > symref. Arguably we'd prefer to tell know if a symref points
>> 
>> s/tell//
>
> Right, thank you.
>
> -Peff

Thanks.  Already tweaked in.

RE: [RFE] Add minimal universal release management capabilities to GIT

[Randall S. Becker]

-Original Message-
From: git-ow...@vger.kernel.org [mailto:git-ow...@vger.kernel.org] On Behalf 
of.mail...@laposte.net
On October 20, 2017 6:41 AM, nicolas wrote:
To: git@vger.kernel.org
Subject: [RFE] Add minimal universal release management capabilities to GIT

>Git is a wonderful tool, which has transformed how software is created, and 
>made code sharing and reuse, a lot easier (both between human and software 
>tools).


> Please please please add release handling and versioning capabilities to Git 
> itself. Without it some enthusiastic
> Git adopters are on a fast trajectory to unmanageable hash soup states, even 
> if they are not realising it yet, because
> the deleterious side effects of giving up on releases only get clear with 
> time.
> Here is what such capabilities could look like (people on this list can 
> probably invent something better, I don't care as long as something exists).


Nicolas makes some interesting points, and I do suggest looking at the original 
post, but there are more factors to consider when dealing with production-grade 
releases in regulatory environments. And my sincere apologies for what, even in 
my eyes looks like a bit of a soap-box rant. No slight intended, Nicolas.

Possibly most importantly, there are serious distinctions between what is built 
via CI, what is released, and what is installed. Some of these can be answered 
addressed directly by git, but others require convention, or a meta-system 
spanning platforms. I will abbreviate some of this:

Commits being used to initiate CI cycles are typically based on source commit 
ids (Jenkins, as an example uses this as an initiator). In Open Source 
environments, where source is specifically released, this is a perfectly 
reasonable release point requiring no more than the commit id itself. 
Committers tend to add tags for convention to make identification convenient, 
and git describe is really helpful here for generating identifying information 
(I state the obvious here). This is the beginning of wisdom, not the end (to 
mis-paraphrase).

Release commits, which are not explicitly in a one-to-one relationships with 
source commits, are a different matter. Suppose the target of your Jenkins 
build creates a release of objects packaged in some useful form. The release 
and source commits are somehow related in your repository of record (loads of 
ways to do this). However, in multi-platform situations, you are in a 
many-to-one situation, obviously since the changes of the release's hash 
matching between two platform builds approaches zero. Nonetheless, the 
release's commit id is relevant to what gets installed, but it is not 
sufficient for human identification purposes. The tag comes in nicely here, and 
hopefully is propagated from the dependent source commit. This 
release-to-source commit derivation is implicitly required in some regulatory 
environments (financial institutions, FDA, FAA, as examples where this exists 
for some systems).

But once you are in a production (or QA) environment, the actual install 
package contains artifacts from a release and from the environment into which 
the release is being installed and activated. The artifacts themselves can be 
highly dynamic and changeable on a radically different and independent schedule 
from the code drop. I advocate keeping those in separate repositories and they 
make for hellacious merge/distribution rules - particularly if the environments 
are radically different in structure, platform, and capability. The 
relationship between commits here is if anything specifically mutable. In a 
specific way, source and release commits are required to be time reversible in 
production, whereby if an installation fails, there exist in many environments 
requirements to be able to fully undo the install action. This is often 
different from the environment artifacts which can be time-forward constrained 
and reversible only in extreme situations. This separate, at least in my 
experience, tends to drive how releases are managed in production shops.

> So nothing terribly complex, just a lot a small helpers to make releasing 
> easier, less tedious, and cheaper for developers,
> that formalize, automate, and make easier existing practices of mature 
> software projects, making them accessible to
> smaller projects. They would make releasing more predictable and reliable for 
> people deploying the code, and easier
> to consume by higher-level cross-project management tools. That would 
> transform the deployment stage of software
> just like Git already transformed early code writing and autotest stages.

Possibly, but primarily for source releases. Release management and the related 
practices are production functions that do not map particularly well (by 
assertion) to the git command set or functionality. As an underlying mechanism 
to manage the production artifacts, git does wonderfully. But installable 
packages (what they think of as 

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Jeff King]

On Sat, Oct 21, 2017 at 06:49:15AM -0400, Eric Sunshine wrote:

> On Thu, Oct 19, 2017 at 1:49 PM, Jeff King  wrote:
> > The refs_resolve_ref_unsafe() function may return NULL even
> > with a REF_ISSYMREF flag if a symref points to a broken ref.
> > As a result, it's possible for find_shared_symref() to
> > segfault when it passes NULL to strcmp().
> > [...]
> > We can fix this by treating NULL the same as a non-matching
> > symref. Arguably we'd prefer to tell know if a symref points
> 
> s/tell//

Right, thank you.

-Peff

Re: [RFE] Add minimal universal release management capabilities to GIT

[nicolas . mailhot]

- Mail original -
De: "Stefan Beller" 

>> Unfortunately Git is so good more and more developers start to procrastinate 
>> on any activity that happens outside of GIT,
>> starting with cutting releases. The meme "one only needs a git commit hash" 
>> is going strong, even infecting institutions
>> like lwn and glibc (https://lwn.net/SubscriberLink/736429/e5a8ccc85cc8/)

> For release you would want to include more than just "the code" into
> the hash, such as compiler versions, environment variables, the phase
> of the moon, what have you, that may impact the release build.

Yes and no. Yes because you do want to limit failure cases, and no because it's 
very easy to overspecify and block code reuse possibilities. Anyway I don't see 
a strong consensus on how to do those yet, they are very language-specific, and 
the first step is being able to identify other code you depend on which 
requires some sort of release id, which is what my message was about. You can't 
build any compatibility matrix, before being able to name the dimensions of the 
matrix.

> It sounds to me as if you assume that if X, Y, Z were numbers (or
> rather had some order), this can be easily deduced.

It's a lot more easy to use "option foo was introduced in version 2.3.4 and 
takes Y parameters" than "option foo was introduced in commit hash 
#, you have version hash 
$$", good luck.

> The output of git-describe ought to be sufficient for an ordering
> scheme to rely on?

That relies on git access to the repo of every bit of code your computer runs. 
This is not practical past the deployment phase. For deployment the ordering 
needs to be extracted from all the git data so you only need to manipulate 
short human and tool-friendly ids. You need low coupling not the strong 
coupling of git repo access.

>> — hashes are not ranked. You can not guess, looking at a hash, if it 
>> corresponds to a project stability point, or is in a
>> middle of a refactoring sequence, where things are expected to break. 
>> Evaluating every hash of every project you use
>> quickly becomes prohibitive, with the only possible strategy being to just 
>> use the latest commit at a given time and pra
>> (and if you are lucky never never update afterwards unless you have lots of 
>> fixing and testing time to waste).

> That is up to the hash function. One could imagine a hash function
> that generates bit patterns that you can use to obtain an order from.

No that is not up to the hash function. First because hashes are too long to be 
manipulated by humans, and second no hash will ever capture human intent. You 
need an explicit human action to mark "I want others to use this particular 
state of my project because I am confident it is solid".

>> – commit mixing is broken by design.

> In Git terms a repository is the whole universe.
> If you want relationships between different projects, you need to
> include these projects e.g. via subtree or submodules.
> It scales even up to linux distributions (e.g.
> https://github.com/gittup/gittup, includes nethack!)

This is still pre-deployment phase. And I wouldn't qualify this as "full linux 
distro", it's very small scale. If anything it demonstrated than even on a 
smallish perimeter relying on git alone as it stands today is too hard (3 
updates in the whole 2017 year!).

>> One can not adapt the user of a piece of code to changes in this piece of 
>> code before those changes are committed in the
>> first place. There will always be moments where the latest commit of a 
>> project, is incompatible with the latest commit of
>> downsteam users of this project. It is not a problem in developer 
>> environments and automated testers, where you want things >> to break early 
>> and be fixed early. It is a huge problem when you follow the same early 
>> commit push strategy for actual
>> production code, where failures are not just a red light in a build farm 
>> dashboard, but have real-world consequences. And
>> the more interlinked git repositories you pile on one another, the higher 
>> the probability is two commits won't work with
>> one another with failures cascading down

> That is software engineering in general, I am not sure how Git relates
> to this? Any change that you make (with or without utilizing Git) can
> break the downstream world.

It's a lot easier to manage when you have discrete release synchronisation 
point and not just a flow of commits

>> – commits are a bad inter-project synchronisation point. There are too many 
>> of them, they are not ranked, everyone is
>> choosing a different commit to deploy, that effectively kills the network 
>> effects that helped making traditional releases
>> solid (because distributors used the same release state, and could share 
>> feedback and audit results).

> There are different strategies. Relevant open source projects (kernel,
> glibc, git) are 

Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads

[Junio C Hamano]

René Scharfe  writes:

> FWIW, I use "-?" for that everywhere.  I have yet to find a command or
> environment where it does something dangerous.

Yeah, it would have made the world a better place if we made that
choice back in 2008.  If we start a transition to make it so right
now, we might be able to make the world a better place by 2022,
perhaps.  I am not sure if the pain during the transition is worth
it, though.

Re: [PATCH 4/4] worktree: handle broken symrefs in find_shared_symref()

[Eric Sunshine]

On Thu, Oct 19, 2017 at 1:49 PM, Jeff King  wrote:
> The refs_resolve_ref_unsafe() function may return NULL even
> with a REF_ISSYMREF flag if a symref points to a broken ref.
> As a result, it's possible for find_shared_symref() to
> segfault when it passes NULL to strcmp().
> [...]
> We can fix this by treating NULL the same as a non-matching
> symref. Arguably we'd prefer to tell know if a symref points

s/tell//

> to "refs/heads/foo", but "refs/heads/foo" is broken. But
> refs_resolve_ref_unsafe() isn't capable of giving us that
> information, so this is the best we can do.
>
> Signed-off-by: Jeff King 

Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads

[René Scharfe]

Am 20.10.2017 um 07:35 schrieb Junio C Hamano:
> Jeff King  writes:
> 
>>  It seems weird and inconsistent to me that the meaning of "-h"
>> depends on the position and presence of other unrelated options.

The position is relevant with parse-options for *each* flag for a
different reason as well: You can't put a flag after a non-flag.  I
find that more annoying, as it slightly but noticeable slows down
adding a flag to a previous command by requiring to navigate to the
middle of the line.

(I guess I should train myself to use Meta-b for going back one word
more often..)

> I may be biased as every time I think about this one, the first one
> that comes to my mind is how "grep -h" (not "git grep", but GNU
> grep) behaves.  Yes, "-h" means something else, but by itself, the
> command does not make sense, and "ls-remote -h" is an exception to
> the rule: most sane commands do not have a sensible semantics when
> they take only "-h" and nothing else.  And even for ls-remote it is
> true only when you try to be extra lazy and do not say which remote
> you are asking.
> 
> And that is why I think "'cmd -h" and nothing else consistently
> gives help" may overall be positive in usability, than a rule that
> says "'cmd -h' is for short-help unless -h means something else for
> the command".

FWIW, I use "-?" for that everywhere.  I have yet to find a command or
environment where it does something dangerous.

René

Re: [PATCH 2/3] branch: split validate_new_branchname() into two

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> +/*
>> + * Check if a branch 'name' can be created as a new branch; die otherwise.
>> + * 'force' can be used when it is OK for the named branch already exists.
>> + * Return 1 if the named branch already exists; return 0 otherwise.
>> + * Fill ref with the full refname for the branch.
>> + */
>
> I guess it's better to avoid repeating the comments in both the .h and
> .c file as they might quite easily become stale. I would prefer keeping
> it in the header file alone.

True.  I wrote this while designing the code, so the copy in .c file
came first, and then that was copied to .h file; the one in .c file
can go.

> The change was simple by splitting the function into two and calling
> the right function as required at every call site! As far as I could
> see this doesn't seem to be reducing the confusion that the 'attr_only'
> parameter caused.

The confusion primarily was the way the parameter was named.
"forcing" does not have strong connection to marking "this is only
asking about attributes".  And removing that confusion, by
separating the caller and making it explicit that the caller needs
two separate behaviours depending on the condition, and naming the
functions more appropriately (i.e. is this about creating a new one
that must not exist already?), is the focus of this step.

Re: [PATCH 3/3] branch: forbid refs/heads/HEAD

[Junio C Hamano]

Kaartic Sivaraam  writes:

>> The only difference is improved tests where we use show-ref to make
>> sure refs/heads/HEAD does not exist when it shouldn't, exercise
>> update-ref to create and delete refs/heads/HEAD, and also make sure
>> it can be deleted with "git branch -d".
>
> In which case you might also like to ensure that it's possible to
> "rename" the branch with a name "HEAD" to recover from historical
> mistakes.

Perhaps.  I didn't think it was all that needed---as long as you can
delete, you can recreate at the same commit with a more desirable
name, and it is not like users have tons of repositories with
misnamed branches that they need to fix.  The code may already
handle it, or there may need even more code to support the rename; I
didn't check.

>>  int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
>>  {
>>  strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
>> -if (name[0] == '-')
>> +if (*name == '-' || !strcmp(name, "HEAD"))
>>  return -1;
>
> I guess this makes the check for "HEAD" in builtin/branch::cmd_branch()
>  (line 796) redundant. May be it could be removed?

Perhaps.  But I think that is better done as a follow-up "now the
lower level consistently handles, let's remove the extra check that
has become unnecessary" separate patch.

> So, may be the following test could also be added (untested yet),
>
> test_expect_success 'branch -m can rename refs/heads/HEAD' '
>   git update-ref refs/heads/HEAD HEAD^ &&
>   git branch -m HEAD head &&
>   test_must_fail git show-ref refs/heads/HEAD
> '

Yeah, that would be a good material for that separate follow-up
patch.

Thanks.

Re: [PATCH 0/3] a small branch API clean-up

[Junio C Hamano]

Kaartic Sivaraam  writes:

> On Fri, 2017-10-13 at 14:11 +0900, Junio C Hamano wrote:
>> This started as a little clean-up for a NEEDSWORK comment in
>> branch.h, but it ended up adding a rather useful safety feature.
>> 
>
> Part of this series seems to duplicate the work done in part of my
> series that tries to give more useful error messages when renaming a
> branch.
>
> https://public-inbox.org/git/%3c20170919071525.9404-1-kaarticsivaraam91...@gmail.com%3E/
>
> Any reasons for this?

Sorry, but I am not sure what you are asking.  

I was looking at the code, noticed NEEDSWORK comment and worked on
cleaning things up---you seem to feel that I need a reason, or
perhaps even your permission, when I try improving the codebase?
That starts to sound a bit ridiculous.

As different developers are multiple people, it just happens that
areas somebody changes overlap an area somebody else wants to
change.  It's not like anybody owns a piece of source file when s/he
expresses an interest to work on something that may or may not
affect that area.

Re: [Alt. PATCH] ls-remote: deprecate -h as short for --heads

[Jeff King]

On Fri, Oct 20, 2017 at 02:35:36PM +0900, Junio C Hamano wrote:

> I may be biased as every time I think about this one, the first one
> that comes to my mind is how "grep -h" (not "git grep", but GNU
> grep) behaves.  Yes, "-h" means something else, but by itself, the
> command does not make sense, and "ls-remote -h" is an exception to
> the rule: most sane commands do not have a sensible semantics when
> they take only "-h" and nothing else.  And even for ls-remote it is
> true only when you try to be extra lazy and do not say which remote
> you are asking.

Yeah, I have to admit "grep -h" is a lot more compelling, since it
matches normal grep. And I've used "grep -h" many times without thinking
about it, simply because the rule just falls out naturally there
(there's nothing possible to do without a regex argument, so we'd show
the usage either way).

> And that is why I think "'cmd -h" and nothing else consistently
> gives help" may overall be positive in usability, than a rule that
> says "'cmd -h' is for short-help unless -h means something else for
> the command".

Yeah, I was shooting more for "let's avoid assigning -h to things". But
that's not really possible if we want to be consistent with upstream
grep, which is probably more important.

I think you've convinced me.

-Peff

Re: [PATCH 2/1] mention git stash push first in the man page

[Robert P. J. Day]

On Sat, 21 Oct 2017, Jeff King wrote:

> On Fri, Oct 20, 2017 at 04:04:10AM -0400, Robert P. J. Day wrote:
>
> > > > I don't think there's any reason to go slow in marking something as
> > > > deprecated. It's the part where we follow up and remove or change the
> > > > feature that must take a while.
> > >
> > > Makes sense, let me drop it from the synopsis then.
> >
> >   what, exactly, is the oft-referred-to issue with how "git stash
> > save" works that is being addressed with the new syntax of "git stash
> > push"?
>
> "stash save" soaks up all arguments as the stash message, so it's not
> possible to specify pathspecs. "push" uses "-m " for the stash
> message, and can accept pathspecs.

  ah, i knew that, yeah, that's the ticket. :-)

rday

-- 


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

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

Re: [PATCH v2 2/2] mark git stash push deprecated in the man page

[Jeff King]

On Thu, Oct 19, 2017 at 07:33:04PM +0100, Thomas Gummerer wrote:

> 'git stash push' fixes a historical wart in the interface of 'git stash
> save'.  As 'git stash push' has all functionality of 'git stash save',
> with a nicer, more consistent user interface deprecate 'git stash
> save'.  To do this, remove it from the synopsis of the man page, and
> move it to a separate section, stating that it is deprecated.

This looks fine.

> @@ -87,6 +84,10 @@ linkgit:git-add[1] to learn how to operate the `--patch` 
> mode.
>  The `--patch` option implies `--keep-index`.  You can use
>  `--no-keep-index` to override this.
>  
> +save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] 
> [-q|--quiet] []::
> +
> + This option is deprecated in favour of 'git stash push'.
> +

We could possibly go into more detail, like:

  It differs from "stash push" in that it cannot take pathspecs, and any
  non-option arguments form the message.

or something. Since we don't want people to use it, it probably doesn't
matter much. I just wondered if people would peer at the (long) synopsis
line trying to figure out how it's different.

-Peff

Re: [PATCH v2 1/2] replace git stash save with git stash push in the documentation

[Jeff King]

On Thu, Oct 19, 2017 at 07:33:03PM +0100, Thomas Gummerer wrote:

> diff --git a/git-stash.sh b/git-stash.sh
> index 8b2ce9afda..16919277ba 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -267,11 +267,11 @@ push_stash () {
>   # translation of "error: " takes in your language. E.g. 
> in
>   # English this is:
>   #
> - #$ git stash save --blah-blah 2>&1 | head -n 2
> - #error: unknown option for 'stash save': --blah-blah
> - #   To provide a message, use git stash save -- 
> '--blah-blah'
> - eval_gettextln "error: unknown option for 'stash save': 
> \$option
> -   To provide a message, use git stash save -- '\$option'"
> + #$ git stash push --blah-blah 2>&1 | head -n 2
> + #error: unknown option for 'stash push': --blah-blah
> + #   To provide a message, use git stash push -- 
> '--blah-blah'
> + eval_gettextln "error: unknown option for 'stash push': 
> \$option
> +   To provide a message, use git stash push -m '\$option'"

The user message is fixed here, but doesn't the message for translators
need the same treatment?  I guess it's just talking about the spacing,
so it doesn't need to be completely accurate. But it probably makes
sense to update it at the same time.

As an aside, I do kind of wonder if the right advice for "push" is
different than for "save" here. I.e., should it say "to provide a
pathspec that starts with --, use push -- --blah-blah". I'm not sure it
matters that much. Second-guessing what a user meant seems likely to go
wrong (e.g., "--icnlude-untracked" would trigger this message, which is
just silly). But that's largely orthogonal to what you're doing here.

-Peff

Re: [PATCH 2/1] mention git stash push first in the man page

[Jeff King]

On Fri, Oct 20, 2017 at 04:04:10AM -0400, Robert P. J. Day wrote:

> > > I don't think there's any reason to go slow in marking something as
> > > deprecated. It's the part where we follow up and remove or change the
> > > feature that must take a while.
> >
> > Makes sense, let me drop it from the synopsis then.
> 
>   what, exactly, is the oft-referred-to issue with how "git stash
> save" works that is being addressed with the new syntax of "git stash
> push"?

"stash save" soaks up all arguments as the stash message, so it's not
possible to specify pathspecs. "push" uses "-m " for the stash
message, and can accept pathspecs.

-Peff

Re: [PATCH v4] commit: check result of resolve_ref_unsafe

[Jeff King]

On Fri, Oct 20, 2017 at 04:09:30PM +0300, Andrey Okoshkin wrote:

> Add check of the resolved HEAD reference while printing of a commit summary.
> resolve_ref_unsafe() may return NULL pointer if underlying calls of lstat() or
> open() fail in files_read_raw_ref().
> Such situation can be caused by race: file becomes inaccessible to this 
> moment.
> 
> Signed-off-by: Andrey Okoshkin 
> ---
> Hello,
> I think this way is better for user experience:
> * git doesn't crash;
> * warning is shown;
> * commit has been successfully created then it's safe to show a summary 
> message
> with already known information and without resolved HEAD.

I'm on the fence between this and the die_errno() version. Given that
this would basically never happen in practice, I don't think it matters
too much. And that makes me want to just err on the side of simplicity.
But it's not like this is all that complex, either.

-Peff