git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: Junio C Hamano via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, sandals@crustytoothpaste.net,
	steadmon@google.com, jrnieder@gmail.com, peff@peff.net,
	congdanhqx@gmail.com, phillip.wood123@gmail.com,
	sluongng@gmail.com, jonathantanmy@google.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update
Date: Wed, 12 Aug 2020 17:03:32 -0700	[thread overview]
Message-ID: <xmqqo8nf4dkb.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200812231021.GG2965447@google.com> (Emily Shaffer's message of "Wed, 12 Aug 2020 16:10:21 -0700")

Emily Shaffer <emilyshaffer@google.com> writes:

>> +fetch.writeFetchHEAD::
>> +	Setting it to false tells `git fetch` not to write the list
>> +	of remote refs fetched in the `FETCH_HEAD` file directly
>> +	under `$GIT_DIR`.  Can be countermanded from the command
>> +	line with the `--[no-]write-fetch-head` option.  Defaults to
>> +	true.
>
> [jrnieder] Is providing a config option creating more trouble than
> benefit? Is this a burden on script authors that they need to check the
> config state, when instead they could just pass the flag? Or rather,
> because of the config, the script author has to pass the flag explicitly
> anyways.
> [emily] removing the config also clears up the confusion around 'git pull' +
> 'fetch.writeFetchHEAD' I commented on later.

[A meta comment, but I somehow find this format cumbersome to read
and respond to.  Would it be possible to dedup and/or summarize
comments on a single point?]

I do not think "it becomes cumbersome to design interaction between
command line option and configuration" is a valid reason not to add
configuration variable.  It would be a valid reason not to, if we
have a convincing argument why people won't pass the command line
option very often, though, but you'd need to be prepared for a
possible future in which somebody finds a good use case where a
configuration is useful, which means you cannot forever depend on
the lack of configurability to avoid making a proper design of the
interaction between configuration and command line.

As the feature itself is primarily designed for scripts that want to
always disable writing of FETCH_HEAD, I can certainly understand a
short-term/sighted view of not wanting to add configuration, though.

>> @@ -895,7 +902,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
>>  	const char *what, *kind;
>>  	struct ref *rm;
>>  	char *url;
>> -	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head(the_repository);
>> +	const char *filename = (!write_fetch_head
>> +				? "/dev/null"
>> +				: git_path_fetch_head(the_repository));
>
> [emily] Huh. so what does the dry_run codepath look like now? It looks
> like it's been superseded by write_fetch_head but the option parse
> doesn't tell dry_run to update write_fetch_head instead. (Found the
> answer later, see below)
>
>> @@ -1797,6 +1806,10 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>>  	if (depth || deepen_since || deepen_not.nr)
>>  		deepen = 1;
>>  
>> +	/* FETCH_HEAD never gets updated in --dry-run mode */
>> +	if (dry_run)
>> +		write_fetch_head = 0;
>
> [emily] Aha, here is where dry_run informs write_fetch_head. I wonder if
> there's a way to instead tell the options struct that --dry-run ~=
> --write-fetch-head?

I do not know with what meaning you used the symbol "~=".  Dry-run
tells the command to operate differently in a few ways, and one of
the (smaller) ways is not to write the FETCH_HEAD file.  Did you
mean "contains", "includes", etc.?

>> +test_expect_success '--write-fetch-head gets defeated by --dry-run' '
>> +	rm -f .git/FETCH_HEAD &&
>> +	git fetch --dry-run --write-fetch-head . &&
>> +	! test -f .git/FETCH_HEAD
>> +'
>
> [emily] Would it make more sense to make these args mutually exclusive?

At least, give a hint, if you cannot state in concrete words, why
you suspect it might make sense to do things differently.  What use
case do you think an alternative design would help better?  Without
it, you can just get "no" and such an exchange would not be useful
at all to improve the patch.

> [jrnieder] If someone specifies both, then they probably want to say
> "show me what I would write to FETCH_HEAD but don't actually do that" -
> which isn't info that we print anyways, right now.

Do you mean "don't actually write but show it to standard output
instead" or something?  If we were designing --dry-run from scratch
back when there were no "git fetch" command, that would have made
tons of sense, I think ;-) To me, the "--write-fetch-head" option
tells it to "write to the file", and not "write the info to unfixed
destination that is not specified by this option but derived by the
presense of other options".  While the "don't download and show what
would be in FETCH_HEAD" might be a good feature to add, combining
these two options may be a good way to invoke the feature.

>> +test_expect_success 'git pull --no-write-fetch-head fails' '
>> +	mkdir clonedwfh &&
>> +	(cd clonedwfh && git init &&
>> +	test_must_fail git pull --no-write-fetch-head "../parent" >out 2>err &&
>> +	test_must_be_empty out &&
>> +	test_i18ngrep "no-write-fetch-head" err)
>
> Should this be shown as a usage error instead of a die()?

As "pull" does and should not take "--[no-]write-fetch-head" as its
option, I think the above command line deserves an usage error, just
like "git pull --update-head-ok" does (or "git pull --no-such-option"
for that matter).


  reply	other threads:[~2020-08-13  0:03 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-06 16:30 [PATCH 0/9] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget
2020-08-06 16:30 ` [PATCH 1/9] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget
2020-08-12 23:10   ` Emily Shaffer
2020-08-13  0:03     ` Junio C Hamano [this message]
2020-08-13  1:45       ` Jonathan Nieder
2020-08-13  4:37       ` [PATCH v3] " Junio C Hamano
2020-08-14  1:13         ` Derrick Stolee
2020-08-14  1:32           ` Junio C Hamano
2020-08-06 16:30 ` [PATCH 2/9] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-08-12 23:10   ` Emily Shaffer
2020-08-14  1:28     ` Derrick Stolee
2020-08-06 16:30 ` [PATCH 3/9] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-08-12 23:10   ` Emily Shaffer
2020-08-14  1:46     ` Derrick Stolee
2020-08-06 16:30 ` [PATCH 4/9] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-08-06 16:30 ` [PATCH 5/9] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-08-06 16:30 ` [PATCH 6/9] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-08-06 16:30 ` [PATCH 7/9] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-08-06 16:30 ` [PATCH 8/9] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-08-06 17:02   ` Son Luong Ngoc
2020-08-06 18:13     ` Derrick Stolee
2020-08-06 16:30 ` [PATCH 9/9] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-08-18 14:25 ` [PATCH v2 0/9] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 1/9] fetch: optionally allow disabling FETCH_HEAD update Junio C Hamano via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 2/9] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 3/9] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 4/9] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 5/9] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 6/9] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 7/9] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 8/9] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-08-18 14:25   ` [PATCH v2 9/9] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-08-25 18:36   ` [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks Derrick Stolee via GitGitGadget
2020-08-25 18:36     ` [PATCH v3 1/8] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-09-22 23:05       ` Jonathan Tan
2020-08-25 18:36     ` [PATCH v3 2/8] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-09-22 23:09       ` Jonathan Tan
2020-09-24 13:45         ` Derrick Stolee
2020-08-25 18:36     ` [PATCH v3 3/8] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-09-22 23:15       ` Jonathan Tan
2020-09-24 13:51         ` Derrick Stolee
2020-08-25 18:36     ` [PATCH v3 4/8] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-09-22 23:16       ` Jonathan Tan
2020-09-24 13:53         ` Derrick Stolee
2020-08-25 18:36     ` [PATCH v3 5/8] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-08-25 18:36     ` [PATCH v3 6/8] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-09-22 23:26       ` Jonathan Tan
2020-09-24 14:05         ` Derrick Stolee
2020-09-24 22:01           ` Jonathan Tan
2020-08-25 18:36     ` [PATCH v3 7/8] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-08-25 18:36     ` [PATCH v3 8/8] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget
2020-09-22 23:52       ` Jonathan Tan
2020-08-25 20:59     ` [PATCH v3 0/8] Maintenance II: prefetch, loose-objects, incremental-repack tasks Junio C Hamano
2020-08-26 15:15     ` Son Luong Ngoc
2020-08-26 16:21       ` Derrick Stolee
2020-09-25 12:33     ` [PATCH v4 " Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 1/8] maintenance: add prefetch task Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 2/8] maintenance: add loose-objects task Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 3/8] maintenance: create auto condition for loose-objects Derrick Stolee via GitGitGadget
2020-09-25 18:00         ` Junio C Hamano
2020-09-25 18:43           ` Derrick Stolee
2020-09-25 12:33       ` [PATCH v4 4/8] midx: enable core.multiPackIndex by default Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 5/8] midx: use start_delayed_progress() Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 6/8] maintenance: add incremental-repack task Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 7/8] maintenance: auto-size incremental-repack batch Derrick Stolee via GitGitGadget
2020-09-25 12:33       ` [PATCH v4 8/8] maintenance: add incremental-repack auto condition Derrick Stolee via GitGitGadget

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo8nf4dkb.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=congdanhqx@gmail.com \
    --cc=derrickstolee@github.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood123@gmail.com \
    --cc=sandals@crustytoothpaste.net \
    --cc=sluongng@gmail.com \
    --cc=steadmon@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).