Git development
 help / color / mirror / Atom feed
* Re: git/git-scm.com GH Issue Tracker
From: Jeff King @ 2017-02-06 18:49 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Samuel Lijin, git@vger.kernel.org
In-Reply-To: <CACsJy8BCTY=T9f2ac7HUuHA-_RzjaxPHZNPQs9ecBhmsnDuRVQ@mail.gmail.com>

On Mon, Feb 06, 2017 at 05:18:03PM +0700, Duy Nguyen wrote:

> On Mon, Feb 6, 2017 at 1:15 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> > # Irrelevant but someone should take a look
> >
> > 693
> 
> To save people some time (and since i looked at it anyway), this is
> about whether "warning in tree xxx: contains zero-padded file modes:
> from fsck should be a warning or error. It is a warning now even
> though "git -c transfer.fsckobjects=true clone" treats it as an error.
> There are some discussions in the past [1] [2] about this.

The bug that caused the trees is long-fixed. There's a question of
how severity levels should be handled in transfer.fsckObjects. By
default it treats everything as a reason to reject the object. Dscho
added configurable levels a few versions ago. It may be a good idea to
tweak the defaults to something more permissive[1].

> There's also a question "And I failed to find in the documentation if
> transfer.fsckobjects could be disabled per repository, can you confirm
> it's not possible for now ?"

I don't know why it wouldn't be, though note that it won't override
the operation-specific {receive,fetch}.fsckObjects.

-Peff

[1] If we had a more permissive set of defaults, it would probably make
    sense to turn on fsckObjects by default. Some of the checks are
    security-relevant, like disallowing trees with ".GIT",
    "../../etc/passwd", etc. Those _should_ be handled sanely by the
    rest of Git, but it serves as a belt-and-suspenders check, and also
    protects anybody with a buggy Git downstream from you.

    GitHub has had the feature turned on for ages, with a few caveats:

      - we loosened the zero-padded mode warning, because it was causing
	too many false positives

      - we loosened the timezone checks for the same reason; we've seen
	time zones that aren't exactly 4 characters before

      - we occasionally get complaints from people trying to push old
	histories with bogus committer idents. Usually a missing name or
	similar.

     So those are the ones we'd probably need to loosen off the bat, and
     they're all pretty harmless. But it would be a potential irritating
     regression for somebody if they have a history with other minor
     flaws, and Git suddenly starts refusing to clone it.

^ permalink raw reply
* Re: git/git-scm.com GH Issue Tracker
From: Jeff King @ 2017-02-06 18:34 UTC (permalink / raw)
  To: Samuel Lijin; +Cc: git@vger.kernel.org
In-Reply-To: <CAJZjrdX_8tjMhRac9QQOW8m_S2DprFPV=uZp8mFT+g6bASVd-w@mail.gmail.com>

On Mon, Feb 06, 2017 at 12:15:08AM -0600, Samuel Lijin wrote:

> I've went through a bunch of open issues on the git/git-scm.com repo
> (specifically, everything after #600) and I think the bulk of them can
> be closed.
> 
> I've taken the liberty of classifying them as shown below.

Thanks, this is incredibly helpful. I'll close the appropriate ones you
identified.

-Peff

^ permalink raw reply
* Re: [PATCH 00/12] completion: speed up refs completion
From: Jacob Keller @ 2017-02-06 18:31 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: Junio C Hamano, Git mailing list
In-Reply-To: <CA+P7+xpeyebN3pmSX09Avh1EvYVjALpBCQ9r2==q3SWTu3GMSw@mail.gmail.com>

On Fri, Feb 3, 2017 at 7:15 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> I haven't had a chance to further investigate, but I tried this series
> out (from your github) and it appears that this series (or the
> previous series for __gitdir work) breaks "git log" ref completion.
> I'll have further details when I am able to investigate a it more.
>
> Thanks,
> Jake

At first I had the same problem, but I verified by re-installing the
completion script and the problem appears to have gone away. I suspect
what happened is that the original time, I forgot to actually install
the new version of git, and only installed the completion script, so
when some of the commands were run with new options they (silently)
failed and the result was missing completion values.

Once I properly re-installed everything it appears to work as
expected. I haven't found any other issues yet.

Regards,
Jake

^ permalink raw reply
* Re: git-scm.com status report
From: Jeff King @ 2017-02-06 18:27 UTC (permalink / raw)
  To: git
In-Reply-To: <20170202023349.7fopb3a6pc6dkcmd@sigill.intra.peff.net>

On Thu, Feb 02, 2017 at 03:33:50AM +0100, Jeff King wrote:

> We (the Git project) got control of the git-scm.com domain this year. We
> have never really had an "official" website, but I think a lot of people
> consider this to be one.
> 
> This is an overview of the current state, as well as some possible
> issues and future work.

Thanks everybody, for your responses here and off-list. After my mail
got posted to HN, I got quite a lot of private responses, including
offers to sponsor hosting, work on the site, etc. I'm still working my
way through them, but I wanted to try to respond in aggregate here.

First, a few clarifications:

  - The money for the site wasn't mentioned to me by GitHub at all.  I'm
    quite sure they would continue to sponsor the site financially if
    need be. The only reason I didn't promise that is because I hadn't
    arranged it specifically, and "step 0" seemed like first making sure
    our costs were reasonable.

  - Spinning the site out of GitHub's Heroku account isn't an urgent or
    impending change. It came out of a conversation I had with people
    auditing the GitHub account, where it is clearly a funny historical
    anomaly. So I suspect we could just stay there indefinitely if need
    be. But it seems to me like the right thing is to move it out for
    two reasons:

      1. The site was always intended to serve the Git community, not
         GitHub, and it has increasingly become a community asset (e.g.,
	 with the transfer of the domain name). The hosting assets
	 should be held by the community, too, to help with things like
	 continuity. If I get hit by a bus, the rest of the Git PLC
	 should have access to the site without having to figure out who
	 owns what.

      2. Right now I can't add any other co-admins to handle operational
         issues. So the bus factor and load of that part of operating
	 the site can't be spread.

The responses I've gotten fall into a few buckets, I think:

  - Yes, the current hosting cost really is unnecessarily high. Most of
    this is due to scaling wrong. The main costs are:

      1. Using 2x dynos; these have 1GB of RAM versus 512MB. The site
         does seem to use about 750MB. I have no idea why that is the
	 case. There's probably some low-hanging fruit in reducing the
	 memory use to keep it below 512MB, but I don't think anybody
	 has dug in there.

      2. The site is scaled by using 3 dynos. It would be simpler and
         cheaper to stick a CDN in front of it, since the pages change
	 very rarely. That's something I haven't looked into setting up
	 yet.

	 The prerequisite to using a CDN is actually making sure the
	 content is deterministic and cacheable. There was a nice PR
	 opened at https://github.com/git/git-scm.com/pull/941 towards
	 that end.

  - It's mostly silly for this to be a Rails app at all. It's a static
    site which occasionally sucks in and formats new content (like the
    latest git version, new manpages, etc). The intent here was to make
    something that would "just run" forever and pick up new versions
    without human intervention. And that _does_ work, but it also makes
    things more expensive and complicated than they need to be.

    So a viable alternative is to use some kind of static site
    generator and have someone (or something) responsible for pulling in
    the new git versions occasionally.

    A few people have expressed interesting this. There's some
    preliminary work here:

      https://github.com/git/git-scm.com/pull/941

    and at least GitLab has expressed some interest. So I'll let people
    coordinate in that PR or a new one what the result should look like.
    Working patches trump discussion. :)

    I have also talked with the GitHub Pages people, and they think
    hosting it as a Jekyll page wouldn't be a big deal performance-wise
    (with the caveat that we'd need to pre-render the asciidoctor bits
    ourselves, as Jekyll doesn't do asciidoc). So that's a viable option
    for hosting it for effectively free (though I think we _would_ still
    want to put a CDN in front of it). But if somebody has an
    alternative option, that's fine, too.

  - Some people offered to help with running the site, or making major
    transitions (like converting to a static site). The most important
    thing to me there is that we have a solid maintenance plan. So I
    would want some evidence that anybody doing a major work would stick
    around in the community afterwards, or that it be done in a way that
    the handoff back to community members is easy. So I'd probably look
    for somebody already involved in the community, or somebody who
    wants to join it building up that trust by taking on site
    responsibilities over time.

  - Lots of people asked about small tasks to do. Mostly reviewing and
    responding to issues and PR is the simplest thing. You can do it in
    a drive-by way, and that helps take the load off of me. As the same
    reviewers show up more and more, I think we can build a community
    and I'd eventually hand out greater access to the site to match.

    I notice I've got over 100 GitHub notifications from people sifting
    through back-issues, so that will take some time to go through. I'm
    hoping a lot of them are "already fixed, click closed". :)

  - Several people offered money out of pocket to pay for hosting, and
    several hosters contacted me to work out hosting deals ranging from
    cheap to free. I'd prefer to explore the technical bits for now and
    see what the final shape and cost actually is (if we move to a
    non-Rails site, then Rails hosting is less appealing, obviously).

So that's where we're at. I think the next step is either sticking a CDN
in front of Heroku and dialing down the scaling there, or moving to a
static site. I'll probably stall for a bit and see if patches for the
latter materialize, and if not pursue the CDN thing (where most of the
work will be administrative in getting it set up, not technical. I think
that makes it more or less my thing to do, but if anybody is interested
in setting it up and handing off an account to the project, that
certainly makes things easy).

Thanks again for everybody who has offered to help, and everybody who
continues to do so.

-Peff

^ permalink raw reply
* [PATCH] squash! completion: fill COMPREPLY directly when completing refs
From: SZEDER Gábor @ 2017-02-06 18:15 UTC (permalink / raw)
  To: git; +Cc: Junio C Hamano, SZEDER Gábor
In-Reply-To: <20170203025405.8242-13-szeder.dev@gmail.com>

Care should be taken, though, because that prefix might contain
'for-each-ref' format specifiers as part of the left hand side of a
'..' range or '...' symmetric difference notation or fetch/push/etc.
refspec, e.g. 'git log "evil-%(refname)..br<TAB>'.  Doubling every '%'
in the prefix will prevent 'git for-each-ref' from interpolating any
of those contained format specifiers.
---

This is really pathological, and I'm sure this has nothing to do with
whatever breakage Jacob experienced.
The shell metacharacters '(' and ')' still cause us trouble in various
ways, but that's nothing new and has been the case for quite a while
(always?).

It's already incorporated into (the rewritten)

  https://github.com/szeder/git completion-refs-speedup

 contrib/completion/git-completion.bash |  8 +++++---
 t/t9902-completion.sh                  | 11 +++++++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index dbbb62f5f..058f1d0ee 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -381,6 +381,7 @@ __git_refs ()
 	local list_refs_from=path remote="${1-}"
 	local format refs
 	local pfx="${3-}" cur_="${4-$cur}" sfx="${5-}"
+	local fer_pfx="${pfx//\%/%%}"
 
 	__git_find_repo_path
 	dir="$__git_repo_path"
@@ -406,6 +407,7 @@ __git_refs ()
 	if [ "$list_refs_from" = path ]; then
 		if [[ "$cur_" == ^* ]]; then
 			pfx="$pfx^"
+			fer_pfx="$fer_pfx^"
 			cur_=${cur_#^}
 		fi
 		case "$cur_" in
@@ -429,13 +431,13 @@ __git_refs ()
 				"refs/remotes/$cur_*" "refs/remotes/$cur_*/**")
 			;;
 		esac
-		__git_dir="$dir" __git for-each-ref --format="$pfx%($format)$sfx" \
+		__git_dir="$dir" __git for-each-ref --format="$fer_pfx%($format)$sfx" \
 			"${refs[@]}"
 		if [ -n "$track" ]; then
 			# employ the heuristic used by git checkout
 			# Try to find a remote branch that matches the completion word
 			# but only output if the branch name is unique
-			__git for-each-ref --format="$pfx%(refname:strip=3)$sfx" \
+			__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
 				--sort="refname:strip=3" \
 				"refs/remotes/*/$cur_*" "refs/remotes/*/$cur_*/**" | \
 			uniq -u
@@ -457,7 +459,7 @@ __git_refs ()
 			case "HEAD" in
 			$cur_*)	echo "${pfx}HEAD$sfx" ;;
 			esac
-			__git for-each-ref --format="$pfx%(refname:strip=3)$sfx" \
+			__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
 				"refs/remotes/$remote/$cur_*" \
 				"refs/remotes/$remote/$cur_*/**"
 		else
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 1206a38ed..be584c069 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -951,6 +951,17 @@ test_expect_success 'teardown after filtering matching refs' '
 	git update-ref -d refs/remotes/other/matching/branch-in-other
 '
 
+test_expect_success '__git_refs - for-each-ref format specifiers in prefix' '
+	cat >expected <<-EOF &&
+	evil-%%-%42-%(refname)..master
+	EOF
+	(
+		cur="evil-%%-%42-%(refname)..mas" &&
+		__git_refs "" "" "evil-%%-%42-%(refname).." mas >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
 test_expect_success '__git_complete_refs - simple' '
 	sed -e "s/Z$//g" >expected <<-EOF &&
 	HEAD Z
-- 
2.11.1.499.gb6fcc8b3a


^ permalink raw reply related
* Re: [PATCH v3] parse-remote: remove reference to unused op_prep
From: Junio C Hamano @ 2017-02-06 18:17 UTC (permalink / raw)
  To: Siddharth Kannan; +Cc: Pranit Bauva, Git List
In-Reply-To: <20170206022804.GB3323@ubuntu-512mb-blr1-01.localdomain>

Siddharth Kannan <kannan.siddharth12@gmail.com> writes:

> Hey Pranit,
> On Sun, Feb 05, 2017 at 02:45:46AM +0530, Pranit Bauva wrote:
>> Hey Siddharth,
>> 
>> On Sat, Feb 4, 2017 at 8:01 PM, Siddharth Kannan
>> <kannan.siddharth12@gmail.com> wrote:
>> > The error_on_missing_default_upstream helper function learned to
>> > take op_prep argument with 15a147e618 ("rebase: use @{upstream}
>> > if no upstream specified", 2011-02-09), but as of 045fac5845
>> > ("i18n: git-parse-remote.sh: mark strings for translation",
>> >  2016-04-19), the argument is no longer used.  Remove it.
>> >
>> > Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
>> 
>> This looks good to me! Thanks :)
>> 
>> Regards,
>> Pranit Bauva
>
> Should I send this patch with "To:" set to Junio and "Cc:" set to the
> mailing list, as mentioend in the SubmittingPatches document?

Nah, I was watching the discussion from the sideline.  I'll pick it
up after doing one final read on the patch myself.

Thanks, both.

^ permalink raw reply
* Re: [PATCH] difftool: fix bug when printing usage
From: Junio C Hamano @ 2017-02-06 18:13 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: David Aguilar, Git ML, Denton Liu
In-Reply-To: <alpine.DEB.2.20.1702061716120.3496@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> +test_expect_success 'basic usage requires no repo' '
>> +	lines=$(git difftool -h | grep ^usage: | wc -l) &&
>> +	test "$lines" -eq 1 &&
>
> It may be easier to debug future breakages if you wrote
>
> 	git difftool -h | grep ^usage: >output &&
> 	test_line_count = 1 output &&
> or even better (changing the semantics now):
>
> 	test_expect_code 129 git difftool -h >output &&
> 	grep ^usage: output &&

True.

>> +	# create a ceiling directory to prevent Git from finding a repo
>> +	mkdir -p not/repo &&
>> +	ceiling="$PWD/not" &&
>> +	lines=$(cd not/repo &&
>> +		GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
>> +		grep ^usage: | wc -l) &&
>> +	test "$lines" -eq 1 &&
>
> Likewise, this would become
>
> 	GIT_CEILING_DIRECTORIES="$PWD/not" \
> 	test_expect_code 129 git -C not/repo difftool -h >output &&
> 	grep ^usage: output

I agree with the intent, but the execution here is "Not quite".
test_expect_code being a shell function, it does not take the
"one-shot environment assignment for this single invocation," like
external commands do.

> More importantly: When I read $PWD all kinds of alarm bells go off in my
> head, as I immediately think of all the issues we have on Windows due to
> Git's regression test using POSIX paths all over the place.

And we appreciate that somebody who is more familiar with the issue
is watching ;-).

> Insofar as I am the author of the builtin difftool:
>
> Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

OK, thanks.

^ permalink raw reply
* Re: [PATCH/RFC] WIP: log: allow "-" as a short-hand for "previous branch"
From: Siddharth Kannan @ 2017-02-06 18:10 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, pranit.bauva, Matthieu.Moy, peff, pclouds, sandals
In-Reply-To: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>

Hey Junio, I did some more digging into the codepath:

On Sun, Feb 05, 2017 at 04:15:03PM -0800, Junio C Hamano wrote:
> 
> A correct solution needs to know if the argument is at the position
> where a revision (or revision range) is expected and then give the
> tip of the previous branch when it sees "-" (and other combinations
> this patch tries to cover).  In other words, the parser always knows
> what it is parsing, and if and only if it is parsing a rev, react to
> "-" and think "ah, the user wants me to use the tip of the previous
> branch".
> 
> But the code that knows that it expects to see a revision already
> exists, and it is the real parser.  In the above snippet,
> setup_revisions() is the one that does the real parsing of argv[].
> The code there knows when it wants to see a rev, and takes argv[i]
> and turns into an object to call add_pending_object().  That codepath
> may not yet know that "-" means the tip of the previous branch, and
> that is where the change needs to go.

Inside setup_revisions, it tries to parse arguments and options. In
there, is this line of code:

    if (*arg == '-') {

Once control enters this branch, it will either parse the argument as
an option / pseudo-option, or simply leave this argument as is in the
argv[] array and move forward with the other arguments.

So, first I need to teach setup_revisions that something starting with
a "-" might be a revision or a revision range.

After this, going further down the codepath, in
revision.c:handle_revision_arg: 

The argument is parsed to find out if it is of the form
rev1...rev2 or rev1..rev2 or just rev1, etc.

(a) -> If `..` or `...` was found, then two pointers "this" and "next"
now hold the from and to revisions, and the function
get_sha1_committish is called on them. In case both were found to be
committish, then the char pointers now hold the sha1 in them, they are
parsed into objects.

(b) -> Else look for "r1^@", "r1^!" (This could be "-^@", "-^!") To
get r1, again the function get_sha1_committish is called with only r1
as the parameter.

(c) -> Else look for "r1^-"

(d) -> Else look for the argument using the same get_sha1_committish
function (It any "^" was present in it, it has already been noted and
removed)

Cases (a), (b) and (d) can be handled by putting this inside
get_sha1_committish. (Further discussion about that below)

Case (c) is a bit confusing. This could be something like "-^-", and
something like "^-" could mean "Not commits on previous branch" or it
could mean "All commits on this branch except for the parent of HEAD"
Please tell me if this is confusing or undesired altogether.
Personally, I feel that people who have been using "^-" would be
very confused if it's behaviour changed.

So, all the code till now points at adding the patch for "-" = "@{-1}"
inside get_sha1_committish or downstream from there.

get_sha1_committish 
-> get_sha1_with_context 
-> get_sha1_with_context_1
-> get_sha1_1 
  -> peel_onion -> calling get_sha1_basic again with the ref
  only (after peeling) 
  -> get_sha1_basic -> includes parsing of "@{-N}" type revs. So, 
  this indicates that if we can convert the "-" appropriately 
  before this point, then it would be good.
  -> get_short_sha1

So, this patch reduces to the following 2 tasks:

1. Teach setup_revisions that something starting with "-" can be an
argument as well
2. Teach get_sha1_basic that "-" means the tip of the previous branch
perhaps by replacing it with "@{-1}" just before the reflog parsing is
done

> A correct solution will be a lot more involved, of course, and I
> think it will be larger than a reasonable microproject for people
> new to the codebase.

So true :) I had spent a fair bit of time already on my previous patch,
and I thought I might as well complete my research into this, and send
this write-up to the mailing list, so that I could write a patch some
time later. In case you would prefer for me to not work on this
anymore because I am new to the codebase, I will leave it at this.

- Siddharth Kannan

^ permalink raw reply
* Re: [PATCH 00/13] gitk: tweak rendering of remote-tracking references
From: Marc Branchaud @ 2017-02-06 17:13 UTC (permalink / raw)
  To: Michael Haggerty, Paul Mackerras; +Cc: git
In-Reply-To: <cover.1482164633.git.mhagger@alum.mit.edu>

On 2016-12-19 11:44 AM, Michael Haggerty wrote:
> This patch series changes a bunch of details about how remote-tracking
> references are rendered in the commit list of gitk:

I don't see this series in git v2.12.0-rc0, nor in Paul's gitk repo.

I hope this is an oversight, and not that the series got dropped for 
some reason.

		M.



> * Omit the "remote/" prefix on normal remote-tracking references. They
>   are already distinguished via their two-tone rendering and (usually)
>   longer names, and this change saves a lot of visual clutter and
>   horizontal space.
>
> * Render remote-tracking references that have more than the usual
>   three slashes like
>
>       origin/foo/bar
>       ^^^^^^^
>
>   rather than
>
>       origin/foo/bar (formerly remotes/origin/foo/bar)
>       ^^^^^^^^^^^              ^^^^^^^^^^^^^^^^^^^
>
>   , where the indicated part is the prefix that is rendered in a
>   different color. Usually, such a reference represents a remote
>   branch that contains a slash in its name, so the new split more
>   accurately portrays the separation between remote name and remote
>   branch name.
>
> * Introduce a separate constant to specify the background color used
>   for the branch name part of remote-tracking references, to allow it
>   to differ from the color used for local branches (which by default
>   is bright green).
>
> * Change the default background colors for remote-tracking branches to
>   light brown and brown (formerly they were pale orange and bright
>   green).
>
> I understand that the colors of pixels on computer screens is an even
> more emotional topic that that of bikesheds, so I implemented the last
> change as a separate commit, the last one in the series. Feel free to
> drop it if you don't want the default color change.
>
> Along the way, I did a bunch of refactoring in the area to make these
> changes easier, and introduced a constant for the background color of
> "other" references so that it can also be adjusted by users.
>
> (Unfortunately, these colors can only be adjusted by editing the
> configuration file. Someday it would be nice to allow them to be
> configured via the preferences dialog.)
>
> It's been a while since I've written any Tcl code, so I apologize in
> advance for any howlers :-)
>
> This branch applies against the `master` branch in
> git://ozlabs.org/~paulus/gitk.
>
> Michael
>
> Michael Haggerty (13):
>   gitk: when processing tag labels, don't use `marks` as scratch space
>   gitk: keep track of tag types in a separate `types` array
>   gitk: use a type "tags" to indicate abbreviated tags
>   gitk: use a type "mainhead" to indicate the main HEAD branch
>   gitk: fill in `wvals` as the tags are first processed
>   gitk: simplify regexp
>   gitk: extract a method `remotereftext`
>   gitk: only change the color of the "remote" part of remote refs
>   gitk: shorten labels displayed for modern remote-tracking refs
>   gitk: use type "remote" for remote-tracking references
>   gitk: introduce a constant otherrefbgcolor
>   gitk: add a configuration setting `remoterefbgcolor`
>   gitk: change the default colors for remote-tracking references
>
>  gitk | 114 ++++++++++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 76 insertions(+), 38 deletions(-)
>

^ permalink raw reply
* Re: subtree merging fails
From: Johannes Schindelin @ 2017-02-06 17:08 UTC (permalink / raw)
  To: Stavros Liaskos; +Cc: git
In-Reply-To: <CAEXhnEDW_s5vGKmLA8ie63FivFYwtCASyaD_Sowjj3e5U9wp_Q@mail.gmail.com>

Hi Stavros,


On Mon, 6 Feb 2017, Stavros Liaskos wrote:

> Please check this issue for a description of the bug:
> https://github.com/git/git-scm.com/issues/896#issuecomment-277587626

Just an observation from my side: if I see a request for help on this
mailing list where the sender merely pastes a link and does not bother to
condense and summarize the information in the linked page, to help
potential helpers, I am highly unlikely to even click on that link, let
alone to try to help.

Maybe you would want to spend as much effort on a request for help as you
would like to ask people to spend on a reply?

Ciao,
Johannes

^ permalink raw reply
* Re: [PATCH] difftool: fix bug when printing usage
From: Johannes Schindelin @ 2017-02-06 16:58 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Git ML, Denton Liu
In-Reply-To: <20170205212338.17667-1-davvid@gmail.com>

Hi David,

On Sun, 5 Feb 2017, David Aguilar wrote:

> "git difftool -h" reports an error:
> 
> 	fatal: BUG: setup_git_env called without repository
> 
> Defer repository setup so that the help option processing happens before
> the repository is initialized.
> 
> Add tests to ensure that the basic usage works inside and outside of a
> repository.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> This bug exists in both "master" and "next".
> This patch has been tested on both branches.

Thanks for noticing and for patching!

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index b5e85ab079..d13350ce83 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -647,10 +647,6 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
>  		OPT_END()
>  	};
>  
> -	/* NEEDSWORK: once we no longer spawn anything, remove this */
> -	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> -	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> -
>  	git_config(difftool_config, NULL);
>  	symlinks = has_symlinks;
>  
> @@ -661,6 +657,10 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
>  	if (tool_help)
>  		return print_tool_help();
>  
> +	/* NEEDSWORK: once we no longer spawn anything, remove this */
> +	setenv(GIT_DIR_ENVIRONMENT, absolute_path(get_git_dir()), 1);
> +	setenv(GIT_WORK_TREE_ENVIRONMENT, absolute_path(get_git_work_tree()), 1);
> +
>  	if (use_gui_tool && diff_gui_tool && *diff_gui_tool)
>  		setenv("GIT_DIFF_TOOL", diff_gui_tool, 1);
>  	else if (difftool_cmd) {

Aaargh. You know, when you are used to code review systems where you can
easily increase the context of the diff hunks, static patches are...
tedious to review.

For the record: the context between those two hunks is:

       argc = parse_options(argc, argv, prefix, builtin_difftool_options,
                             builtin_difftool_usage, PARSE_OPT_KEEP_UNKNOWN |
                             PARSE_OPT_KEEP_DASHDASH);

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index aa0ef02597..21e2ac4ad6 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -23,6 +23,19 @@ prompt_given ()
>  	test "$prompt" = "Launch 'test-tool' [Y/n]? branch"
>  }
>  
> +test_expect_success 'basic usage requires no repo' '
> +	lines=$(git difftool -h | grep ^usage: | wc -l) &&
> +	test "$lines" -eq 1 &&

It may be easier to debug future breakages if you wrote

	git difftool -h | grep ^usage: >output &&
	test_line_count = 1 output &&

or even better (changing the semantics now):

	test_expect_code 129 git difftool -h >output &&
	grep ^usage: output &&

> +	# create a ceiling directory to prevent Git from finding a repo
> +	mkdir -p not/repo &&
> +	ceiling="$PWD/not" &&
> +	lines=$(cd not/repo &&
> +		GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
> +		grep ^usage: | wc -l) &&
> +	test "$lines" -eq 1 &&

Likewise, this would become

	GIT_CEILING_DIRECTORIES="$PWD/not" \
	test_expect_code 129 git -C not/repo difftool -h >output &&
	grep ^usage: output

> +	rmdir -p not/repo

I am not sure how portable `rmdir -p` is. Why not use

	test_when_finished rm -r not

instead?

But those are all really minor bike-sheddings, and I promised myself that
I would avoid those (as I find them rather pointless) unless there is at
least one real benefit. In this case, I think the result becomes more
readable:

-- snip --
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 21e2ac4ad6d..7d7cb63d61e 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -24,16 +24,14 @@ prompt_given ()
 }
 
 test_expect_success 'basic usage requires no repo' '
-	lines=$(git difftool -h | grep ^usage: | wc -l) &&
-	test "$lines" -eq 1 &&
-	# create a ceiling directory to prevent Git from finding a repo
-	mkdir -p not/repo &&
-	ceiling="$PWD/not" &&
-	lines=$(cd not/repo &&
-		GIT_CEILING_DIRECTORIES="$ceiling" git difftool -h |
-		grep ^usage: | wc -l) &&
-	test "$lines" -eq 1 &&
-	rmdir -p not/repo
+	test_expect_code 129 git difftool -h >output &&
+	grep ^usage: output &&
+	# create a ceiling directory to prevent Git from finding a repo
+	mkdir -p not/repo &&
+	test_when_finished rm -r not &&
+	GIT_CEILING_DIRECTORIES="$PWD/not" \
+	test_expect_code 129 git -C not/repo difftool -h >output &&
+	grep ^usage: output
 '
 
 # Create a file on master and change it on branch
-- snap --

More importantly: When I read $PWD all kinds of alarm bells go off in my
head, as I immediately think of all the issues we have on Windows due to
Git's regression test using POSIX paths all over the place.

In this particular instance, it *does* work, magically, because the POSIX
path $PWD is automatically converted (by the MSYS2 runtime) into a Windows
path when git.exe is called.

Read: I actually tested this, in line with my resolution to avoid to
review merely patches (which would likely miss serious bugs) and instead
try to do full code reviews, including testing. That means that I will
review less, but better.

Insofar as I am the author of the builtin difftool:

Acked-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks,
Dscho

^ permalink raw reply related
* Re: [PATCH] tag: generate useful reflog message
From: Cornelius Weig @ 2017-02-06 16:54 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, bitte.keine.werbung.einwerfen, karthik.188, peff
In-Reply-To: <xmqqo9yg43uo.fsf@gitster.mtv.corp.google.com>


On 02/06/2017 12:25 AM, Junio C Hamano wrote:
> cornelius.weig@tngtech.com writes
> For a tag, I would imagine something like "tag: tagged 4e59582ff7
> ("Seventh batch for 2.12", 2017-01-23)" would be more appropriate.

Yes, I agree that this is much clearer. The revised version v3
implements this behavior.

>> Notes:
>>     While playing around with tag reflogs I also found a bug that was present
>>     before this patch. It manifests itself when the sha1-ref in the reflog does not
>>     point to a commit object but something else.
> 
> I think the fix would involve first ripping out the "reflog walking"
> code that was bolted on and stop allowing it to inject the entries
> taken from the reflog into the "walk the commit DAG" machinery.
> Then "reflog walking" code needs to be taught to have its own "now
> we got a single object to show, show it (using the helper functions
> to show a single object that is already used by 'git show')" code,
> instead of piggy-backing on the output codepath used by "log" and
> "rev-list".

I'll start investigating how that could be done. My first glance tells
me that it won't be easy. Especially because I'm not yet familiar with
the git code.

Thanks for your advice!

^ permalink raw reply
* Re: feature request: add -q to "git branch"
From: Kevin Layer @ 2017-02-06 16:39 UTC (permalink / raw)
  To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPyAxeU2i-OL62O749GaTdL7H19jbbAj8R6fipVnUjt=Q@mail.gmail.com>

I think I got my git versions (old and new) mixed up.  Sorry for the noise.

On Sat, Feb 4, 2017 at 1:17 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Hey Kevin,
>
> Sorry for the previous message.
>
> On Sun, Feb 5, 2017 at 2:47 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Hey Kevin,
>>
>> On Fri, Feb 3, 2017 at 11:59 PM, Kevin Layer <layer@known.net> wrote:
>>> It should be possible to quietly create a branch.
>
> I think `git branch` is already quiet. Are you seeing something else?
>
> Regards,
> Pranit Bauva

^ permalink raw reply
* Re: BUG: "git checkout -q -b foo origin/foo" is not quiet
From: Kevin Layer @ 2017-02-06 16:38 UTC (permalink / raw)
  To: Cornelius Weig; +Cc: git
In-Reply-To: <3bd29833-8fb2-5097-f735-6a3f61e678bf@tngtech.com>

Yeah, my bad.  I was confused.  Sorry for the noise.

On Fri, Feb 3, 2017 at 4:55 PM, Cornelius Weig
<cornelius.weig@tngtech.com> wrote:
> On 02/03/2017 10:36 PM, Kevin Layer wrote:
>> Note that git version 1.8.3.1 is quiet and does not print the
>> "tracking remote" message.
>
> So what you are saying is, that git v1.8.3.1 *is* quiet, but v1.7.1 is
> not? Sounds like a fixed bug to me.
>
> I also checked with the latest version and it did not print anything
> when used with -q.
>
>
> You seem to urgently need quiet branch creation. Have you thought about
> dumping unwanted output in the shell? E.g. in bash
>
> $ git whatever >&/dev/null

^ permalink raw reply
* Re: git-scm.com status report
From: Jeff King @ 2017-02-06 16:24 UTC (permalink / raw)
  To: Pranit Bauva; +Cc: Git List
In-Reply-To: <CAFZEwPPX4LngKrOHxgEp4aMGKOs0w4LHUBKumtmeRJSZ2_iV_Q@mail.gmail.com>

On Mon, Feb 06, 2017 at 01:41:04AM +0530, Pranit Bauva wrote:

> On Thu, Feb 2, 2017 at 8:03 AM, Jeff King <peff@peff.net> wrote:
> > ## What's on the site
> >
> > We have the domains git-scm.com and git-scm.org (the latter we've had
> > for a while). They both point to the same website, which has general
> > information about Git, including:
> 
> Since we have an "official" control over the website, shouldn't we be
> using the .org domain more because we are more of an organization?
> What I mean is that in many places, we have referred to git-scm.com,
> which was perfectly fine because it was done by github which is a
> company but now I think it would be more appropriate to use
> git-scm.org domain. We can forward all .com requests to .org and try
> to move all reference we know about, to .org. What do you all think?

I don't have a preference myself. I know a lot of non-commercial groups
(which I think the Git project is) try to prefer ".org" to signal that.

Switching it around would require some DNS changes. I think ".org" goes
to a server the DNS provider (Gandi) runs which issues an HTTP 301 to
".com". So we'd want to reverse that, or possibly just treat them both
as equals. That shouldn't be too hard, and will have to be done via
Conservancy.

I don't know what it would mean in terms of search-engine optimization.
I know Google tries to detect duplicate names for sites and treat one as
canonical. And that's going to be ".com" now, based on the existing
redirect and on the fact that most people will have linked to .com.

I'm not sure what disadvantages there are to switching now, or if there
are things we should be doing to tell search engines (I seem to recall
Google's Webmaster tools have options to say "this is the canonical
name"). This is pretty far outside my area of expertise, so it may not
even be something to care about at all. Just things to consider (and
hopefully more clueful people than I can comment on it).

-Peff

^ permalink raw reply
* Re: [PATCH v3 0/5] stash: support pathspec argument
From: Jeff King @ 2017-02-06 16:14 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-1-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:37PM +0000, Thomas Gummerer wrote:

> Thanks Junio for the review in the previous round.
> 
> Changes since v2:
> 
> - $IFS should now be supported by using "$@" everywhere instead of using
>   a $files variable.
> - Added a new patch showing the old behaviour of git stash create is
>   preserved.
> - Rephrased the documentation
> - Simplified the option parsing in save_stash, by leaving the
>   actual parsing to push_stash instead.

Overall, I like the direction this is heading. I raised a few issues,
the most major of which is whether we want to allow the minor regression
in "git stash create -m foo".

This also makes "git stash push -p <pathspec...>" work, which is good. I
don't think "git stash -p <pathspec...>" does, though. I _think_ it
would be trivial to do on top, since we already consider that case an
error. That's somewhat outside the scope of your series, so I won't
complain (too much :) ) if you don't dig into it, but it might just be
trivial on top.

A few other random bits I noticed while playing with the new code:

  $ git init
  $ echo content >file && git add file && git commit -m file
  $ echo change >file

  $ git stash push -p not-file
  No changes.
  No changes selected

Probably only one of those is necessary. :)

Let's keep trying a few things:

  $ git stash push not-file
  Saved working directory and index state WIP on master: 5d5f951 file
  Unstaged changes after reset:
  M	file
  M	file

The unstaged change is mentioned twice, which is weird. But weirder
still is that we created a stash at all, as it's empty. Why didn't we
hit the "no changes selected" path?

And one more:

  $ echo foo >untracked
  $ git stash push untracked
  Saved working directory and index state WIP on master: 5d5f951 file
  Unstaged changes after reset:
  M	file
  M	file
  Removing untracked

We removed the untracked file, even though it wasn't actually stashed! I
thought at first this was because it was mentioned as a pathspec, but it
seems to happen even with a different name:

  $ echo foo >untracked
  $ git stash push does-not-exist
  ...
  Removing untracked

That seems dangerous.

-Peff

^ permalink raw reply
* Re: [PATCH v3 4/5] stash: introduce new format create
From: Jeff King @ 2017-02-06 15:56 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-5-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:41PM +0000, Thomas Gummerer wrote:

> git stash create currently supports a positional argument for adding a
> message.  This is not quite in line with how git commands usually take
> comments (using a -m flag).
> 
> Add a new syntax for adding a message to git stash create using a -m
> flag.  This is with the goal of deprecating the old style git stash
> create with positional arguments.
> 
> This also adds a -u argument, for untracked files.  This is already used
> internally as another positional argument, but can now be used from the
> command line.

How do we tell the difference between new-style invocations, and
old-style ones that look new-style? IOW, I think:

  git stash create -m works

currently treats "-m works" as the full message, and it would now become
just "works".

That may be an acceptable loss for the benefit we are getting. The
alternative is to make yet another verb for create, as we did with
save/push). I have a feeling that hardly anybody uses "create", though,
and it's mostly an implementation detail. So given the obscure nature,
maybe it's an acceptable level of regression. I dunno.

But either way, it should probably be in the commit message in case
somebody does have to track it down later.

-Peff

^ permalink raw reply
* Re: [PATCH v3 3/5] stash: add test for the create command line arguments
From: Jeff King @ 2017-02-06 15:50 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-4-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:40PM +0000, Thomas Gummerer wrote:

> +test_expect_success 'create stores correct message' '
> +	>foo &&
> +	git add foo &&
> +	STASH_ID=$(git stash create "create test message") &&
> +	echo "On master: create test message" >expect &&
> +	git show --pretty=%s ${STASH_ID} | head -n1 >actual &&
> +	test_cmp expect actual
> +'

This misses failure exit codes from "git show" because it's on the left
side of a pipe. Perhaps "git show -s" or "git log -1" would get you the
same output without needing "head" (and saving a process and the time
spent generating the diff, as a bonus).

Ditto in the other tests from this patch and later ones.

-Peff

^ permalink raw reply
* Re: [PATCH v3 2/5] stash: introduce push verb
From: Jeff King @ 2017-02-06 15:46 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-3-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:

> +		-m|--message)
> +			shift
> +			stash_msg=${1?"-m needs an argument"}
> +			;;

I think this is our first use of the "?" parameter expansion magic. It
_is_ in POSIX, so it may be fine. We may get complaints from people on
weird shell variants, though. If that's the only reason to avoid it, I'd
be inclined to try it and see, as it is much shorter.

OTOH, most of the other usage errors call usage(), and this one doesn't.
Nor is the error message translatable. Perhaps we should stick to the
longer form (and add a helper function if necessary to reduce the
boilerplate).

> +save_stash () {
> +	push_options=
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		--help)
> +			show_help
> +			;;
> +		--)
> +			shift
> +			break
> +			;;
> +		-*)
> +			# pass all options through to push_stash
> +			push_options="$push_options $1"
> +			;;
> +		*)
> +			break
> +			;;
> +		esac
> +		shift
> +	done

I suspect you could just let "--help" get handled in the pass-through
case (it generally takes precedence over errors found in other options,
but I do not see any other parsing errors that could be found by this
loop). It is not too bad to keep it, though (the important thing is that
we're not duplicating all of the push_stash options here).

> +	if test -z "$stash_msg"
> +	then
> +		push_stash $push_options
> +	else
> +		push_stash $push_options -m "$stash_msg"
> +	fi

Hmm. So $push_options is subject to word-splitting here. That's
necessary to split the options back apart. It does the wrong thing if
any of the options had spaces in them. But I don't think there are any
valid options which do so, and "save" would presumably not grow any new
options (they would go straight to "push").

So there is a detectable behavior change:

  [before]
  $ git stash "--bogus option"
  error: unknown option for 'stash save': --bogus option
         To provide a message, use git stash save -- '--bogus option'
  [etc...]

  [after]
  $ git stash "--bogus option"
  error: unknown option for 'stash save': --bogus
         To provide a message, use git stash save -- '--bogus'

but it's probably an acceptable casualty (the "right" way would be to
shell-quote everything you stuff into $push_options and then eval the
result when you invoke push_stash).

Likewise, it's usually a mistake to just stick a new option (like "-m")
after a list of unknown options. But it's OK here because we know we
removed any "--" or non-option arguments.

-Peff

^ permalink raw reply
* Cross-referencing the Git mailing list archive with their corresponding commits in `pu`
From: Johannes Schindelin @ 2017-02-06 15:34 UTC (permalink / raw)
  To: Josh Triplett; +Cc: git

Hi Josh,

as discussed at the GitMerge, I am trying to come up with tooling that
will allow for substantially less tedious navigation between the local
repository, the mailing list, and what ends up in the `pu` branch.

That tooling would *still* not help lowering the barrier of entry for
contributing to Git by a lot, as it would *still* not address the problem
that mails sent from the most prevalent desktop mail client, as well as
mails sent from the most prevalent web mail client, are simply and
unceremoniously dropped. (This problem was acknowledged by quite a few
nods even at the Contributors' Summit...) But still, we decided to start
*somewhere* and this tooling is what we agreed on.

It is quite a bit harder going than I would like: as we have figured out,
the Subject: line is not a good way to link the commits with the original
mails containing the patches, as commit messages are modified before being
pushed often enough to make this a fragile matching.

So I thought maybe the From: line (from the body, if available, otherwise
from the header) in conjunction with the "Date:" header would work. But a
preliminary study shows that there are 336 From: + Date: combinations in
the Git mailing list archive that are not unique. 71 of these are shared
by three or more mails, even, and 9 are shared by more than 10 mails,
respectively. This is bad!

Unsurprisingly, the top 10 of these cases were obviously caused by the
builtin `git am` bug where it would not reset the author date properly.
Surprisingly, though, there were a few cases from 2005, too.

I had a quick look to find out what was the culprit (looking at the
17-strong patch series "Documentation fixes in response to my previous
listing" by Nikolai Weibull, but I am at a loss there: the mail claims to
be sent by git-send-email and the patches appear to be generated by
git-format-patch as of v0.99.9l, neither of which had a Date:-related bug
back in that time frame. My best guess is that the patches were mishandled
by a tool similar to rebase -i (which entered Git only at v1.5.3).

For details, see:
http://public-inbox.org/git/11340844841342-git-send-email-mailing-lists.git@rawuncut.elitemail.org/
(this is also an example where public-inbox' thread detection went utterly
wrong, including way too many mails in the "thread")

There was even a case of duplicated Date: headers in 2012. Now, this case
is very curious, as there have been 7 mails with identical Date: header,
but it was not a 6-strong patch series. Instead, it was a 4-strong patch
series that needed three iterations before it was accepted, and the
identical Date: header appears only in v2's patches (*not* in its cover
letter) and it *disappeared* in v3's 4/4, where it was set *back* by a
week (to the Date: it had in v1).

For details, see
http://public-inbox.org/git/cover.1354693001.git.Sebastian.Leske@sleske.name/
and
http://public-inbox.org/git/cover.1354324110.git.Sebastian.Leske@sleske.name/
and
http://public-inbox.org/git/b115a546fa783b4121d118bb8fdb9270443f90fa.1353691892.git.Sebastian.Leske@sleske.name/

This last example also demonstrates a very curious test case for a
different difficulty in trying to reconstruct lost correspondences: the
patch series was applied *twice*, independently of each other. First, on
the day v3 was submitted, it was applied on top of v1.8.1-rc0 (as commits
ee26a6e2b8..dd465ce66f), although it was not merged until v1.8.1-rc3. 22
days later, it was reapplied on top of maint so it could enter v1.8.0.3
(back then, Git still had "patchlevel" versions): c2999adcd5..008c208c2c.

As you can see, there is a many-to-many relationship here, even if you do
leave the *original* branch out of the picture entirely.

Will keep you posted,
Dscho

P.S.: I used public-inbox.org links instead of commit references to the
Git repository containing the mailing list archive, because the format of
said Git repository is so unfavorable that it was determined very quickly
in a discussion between Patrick Reynolds (GitHub) and myself that it would
put totally undue burden on GitHub to mirror it there (compare also Carlos
Nieto's talk at GitMerge titled "Top Ten Worst Repositories to host on
GitHub").

^ permalink raw reply
* Re: [PATCH v3 1/5] Documentation/stash: remove mention of git reset --hard
From: Jeff King @ 2017-02-06 15:22 UTC (permalink / raw)
  To: Thomas Gummerer
  Cc: git, Stephan Beyer, Junio C Hamano, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <20170205202642.14216-2-t.gummerer@gmail.com>

On Sun, Feb 05, 2017 at 08:26:38PM +0000, Thomas Gummerer wrote:

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..2e9e344cd7 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -47,8 +47,9 @@ OPTIONS
>  
>  save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q|--quiet] [<message>]::
>  
> -	Save your local modifications to a new 'stash', and run `git reset
> -	--hard` to revert them.  The <message> part is optional and gives
> +	Save your local modifications to a new 'stash' and roll them
> +	back to HEAD (in the working tree and in the index).
> +	The <message> part is optional and gives

Nice. I think what you ended up with here is concise and accurate.

-Peff

^ permalink raw reply
* Re: [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec
From: Jeff King @ 2017-02-06 15:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Gummerer, git, Stephan Beyer, Marc Strapetz,
	Johannes Schindelin, Øyvind A . Holm, Jakub Narębski
In-Reply-To: <xmqqmvdz3ied.fsf@gitster.mtv.corp.google.com>

On Sun, Feb 05, 2017 at 11:09:14PM -0800, Junio C Hamano wrote:

> > @@ -99,6 +104,10 @@ create_stash () {
> >  	if test -z "$new_style"
> >  	then
> >  		stash_msg="$*"
> > +		while test $# != 0
> > +		do
> > +			shift
> > +		done
> >  	fi
> 
> The intent is correct, but I would probaly empty the "$@" not with
> an iteration of shifts but with a single
> 
> 	set x && shift
> 
> ;-)

Perhaps it is not portable, but I think "set --" does the same thing and
is perhaps less obscure.

-Peff

^ permalink raw reply
* Re: [PATCH] p5302: create repositories for index-pack results explicitly
From: Jeff King @ 2017-02-06 14:41 UTC (permalink / raw)
  To: René Scharfe; +Cc: Junio C Hamano, Git List
In-Reply-To: <251bdc20-19a7-9a6c-9f5a-c7e661810c70@web.de>

On Sun, Feb 05, 2017 at 12:43:29PM +0100, René Scharfe wrote:

> Before 7176a314 (index-pack: complain when --stdin is used outside of a
> repo) index-pack silently created a non-existing target directory; now
> the command refuses to work unless it's used against a valid repository.
> That causes p5302 to fail, which relies on the former behavior.  Fix it
> by setting up the destinations for its performance tests using git init.

Ah, right. Thanks for catching this.

I think p5302 was wrong to rely on the old behavior, and your patch is
the right fix.

-Peff

^ permalink raw reply
* [PATCH v3] tag: generate useful reflog message
From: cornelius.weig @ 2017-02-06 13:58 UTC (permalink / raw)
  To: git; +Cc: Cornelius Weig, karthik.188, peff, gitster
In-Reply-To: <xmqqo9yg43uo.fsf@gitster.mtv.corp.google.com>

From: Cornelius Weig <cornelius.weig@tngtech.com>

When tags are created with `--create-reflog` or with the option
`core.logAllRefUpdates` set to 'always', a reflog is created for them.
So far, the description of reflog entries for tags was empty, making the
reflog hard to understand. For example:
6e3a7b3 refs/tags/test@{0}:

Now, a reflog message is generated when creating a tag, following the
pattern "tag: tagging <short-sha1> (<description>)". If
GIT_REFLOG_ACTION is set, the message becomes "$GIT_REFLOG_ACTION
(<description>)" instead. If the tag references a commit object, the
description is set to the subject line of the commit, followed by its
commit date. For example:
6e3a7b3 refs/tags/test@{0}: tag: tagging 6e3a7b3398 (Git 2.12-rc0, 2017-02-03)

If the tag points to a tree/blob/tag objects, the following static
strings are taken as description:

 - "tree object"
 - "blob object"
 - "other tag object"

Signed-off-by: Cornelius Weig <cornelius.weig@tngtech.com>
---
 builtin/tag.c  | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t7004-tag.sh | 16 +++++++++++++++-
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a9..638b68e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -302,6 +302,54 @@ static void create_tag(const unsigned char *object, const char *tag,
 	}
 }
 
+static void create_reflog_msg(const unsigned char *sha1, struct strbuf *sb)
+{
+	enum object_type type;
+	struct commit *c;
+	char *buf;
+	unsigned long size;
+	int subject_len = 0;
+	const char *subject_start;
+
+	char *rla = getenv("GIT_REFLOG_ACTION");
+	if (rla) {
+		strbuf_addstr(sb, rla);
+	} else {
+		strbuf_addstr(sb, _("tag: tagging "));
+		strbuf_add_unique_abbrev(sb, sha1, DEFAULT_ABBREV);
+	}
+
+	strbuf_addstr(sb, " (");
+	type = sha1_object_info(sha1, NULL);
+	switch (type) {
+	default:
+		strbuf_addstr(sb, _("internal object"));
+		break;
+	case OBJ_COMMIT:
+		c = lookup_commit_reference(sha1);
+		buf = read_sha1_file(sha1, &type, &size);
+		if (!c || !buf) {
+			die(_("commit object %s could not be read"),
+				sha1_to_hex(sha1));
+		}
+		subject_len = find_commit_subject(buf, &subject_start);
+		strbuf_insert(sb, sb->len, subject_start, subject_len);
+		strbuf_addf(sb, ", %s", show_date(c->date, 0, DATE_MODE(SHORT)));
+		free(buf);
+		break;
+	case OBJ_TREE:
+		strbuf_addstr(sb, _("tree object"));
+		break;
+	case OBJ_BLOB:
+		strbuf_addstr(sb, _("blob object"));
+		break;
+	case OBJ_TAG:
+		strbuf_addstr(sb, _("other tag object"));
+		break;
+	}
+	strbuf_addch(sb, ')');
+}
+
 struct msg_arg {
 	int given;
 	struct strbuf buf;
@@ -335,6 +383,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf ref = STRBUF_INIT;
+	struct strbuf reflog_msg = STRBUF_INIT;
 	unsigned char object[20], prev[20];
 	const char *object_ref, *tag;
 	struct create_tag_options opt;
@@ -494,6 +543,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	else
 		die(_("Invalid cleanup mode %s"), cleanup_arg);
 
+	create_reflog_msg(object, &reflog_msg);
+
 	if (create_tag_object) {
 		if (force_sign_annotate && !annotate)
 			opt.sign = 1;
@@ -504,7 +555,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	if (!transaction ||
 	    ref_transaction_update(transaction, ref.buf, object, prev,
 				   create_reflog ? REF_FORCE_CREATE_REFLOG : 0,
-				   NULL, &err) ||
+				   reflog_msg.buf, &err) ||
 	    ref_transaction_commit(transaction, &err))
 		die("%s", err.buf);
 	ref_transaction_free(transaction);
@@ -514,5 +565,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&err);
 	strbuf_release(&buf);
 	strbuf_release(&ref);
+	strbuf_release(&reflog_msg);
 	return 0;
 }
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 072e6c6..3c4cb58 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -80,10 +80,24 @@ test_expect_success 'creating a tag using default HEAD should succeed' '
 	test_must_fail git reflog exists refs/tags/mytag
 '
 
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
 test_expect_success 'creating a tag with --create-reflog should create reflog' '
 	test_when_finished "git tag -d tag_with_reflog" &&
 	git tag --create-reflog tag_with_reflog &&
-	git reflog exists refs/tags/tag_with_reflog
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
+'
+
+git log -1 > expected \
+	--format="format:tag: tagging %h (%s, %cd)%n" --date=format:%F
+test_expect_success 'annotated tag with --create-reflog has correct message' '
+	test_when_finished "git tag -d tag_with_reflog" &&
+	git tag -m "annotated tag" --create-reflog tag_with_reflog &&
+	git reflog exists refs/tags/tag_with_reflog &&
+	sed -e "s/^.*\t//" .git/logs/refs/tags/tag_with_reflog > actual &&
+	test_cmp expected actual
 '
 
 test_expect_success '--create-reflog does not create reflog on failure' '
-- 
2.10.2


^ permalink raw reply related
* [PATCH] worktree: fix option descriptions for `prune`
From: Patrick Steinhardt @ 2017-02-06 13:13 UTC (permalink / raw)
  To: git; +Cc: Patrick Steinhardt, Nguyễn Thái Ngọc Duy, ps

The `verbose` and `expire` options of the `git worktree prune`
subcommand have wrong descriptions in that they pretend to relate to
objects. But as the git-worktree(1) correctly states, these options have
nothing to do with objects but only with worktrees. Fix the description
accordingly.

Signed-off-by: Patrick Steinhardt <patrick.steinhardt@elego.de>
---
 builtin/worktree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9a97e37a3..831fe058a 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -125,9 +125,9 @@ static int prune(int ac, const char **av, const char *prefix)
 {
 	struct option options[] = {
 		OPT__DRY_RUN(&show_only, N_("do not remove, show only")),
-		OPT__VERBOSE(&verbose, N_("report pruned objects")),
+		OPT__VERBOSE(&verbose, N_("report pruned working trees")),
 		OPT_EXPIRY_DATE(0, "expire", &expire,
-				N_("expire objects older than <time>")),
+				N_("expire working trees older than <time>")),
 		OPT_END()
 	};
 
-- 
2.11.1


^ permalink raw reply related
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox