Re: [PATCH 1/2] sha1_name: don't trigger detailed diagnosis for file arguments

Matthieu Moy [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

Matthieu Moy [off-list ref] writes:

quoted

My version reads as

  try something;
  if (it failed && I'm only here to report an error)
          report_error();

which I find easier to understand.

I agree that _this_ part is easy to understand when written that
way.  But then shouldn't there be a blanket "The caller is here only
to report an error, but all the previous code didn't find any error,
so there is something wrong" check much later in the code before it
returns a success?  Or am I being too paranoid?

Currently, there's no problem if get_sha1_with_context_1 returns without
dying, because the caller does:

	if (!(arg[0] == ':' && !isalnum(arg[1])))
		/* try a detailed diagnostic ... */
		get_sha1_with_mode_1(arg, sha1, &mode, 1, prefix);

	/* ... or fall back the most general message. */
	die("ambiguous argument '%s': unknown revision or path not in the working tree.\n"
	    "Use '--' to separate paths from revisions", arg);

If we failed to give a nice diagnosis, we give the generic error
message.

We could add this check within get_sha1_with_context_1 to simplify the
task of the caller, but that would require adding it before each
"return" statement, which I think is overkill.

OK.